Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove incorrect os.chdir, handle relative extends/includes #1124

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

notdian
Copy link
Contributor

@notdian notdian commented Jan 27, 2025

Running docker-compose -f containers/base resulted in a FileNotFoundError. However, it worked after cd containers and then running docker-compose -f base. It appears that the removal of chdir operation was overlooked.

@notdian
Copy link
Contributor Author

notdian commented Jan 27, 2025

Seems like this broke integration tests, maybe the fix should be to make all files absolute before the loop?

@p12tic
Copy link
Collaborator

p12tic commented Jan 28, 2025

Seems like this broke integration tests, maybe the fix should be to make all files absolute before the loop?

Probably. This needs checking.

@notdian notdian force-pushed the file_not_found_error branch 5 times, most recently from 838765e to 869aaef Compare January 28, 2025 15:50
@notdian notdian changed the title remove incorrect os.chdir call to fix folder error remove incorrect os.chdir, handle relative extends/includes Jan 28, 2025
@notdian
Copy link
Contributor Author

notdian commented Jan 28, 2025

@p12tic looks like the issue was with handling of relative imports, used absolute paths for services and includes, tests are passing now

@p12tic
Copy link
Collaborator

p12tic commented Jan 28, 2025

Looks good in principle, thanks!

Several small issues:

  • Could the PR be split into separate commits for each individual piece (includes, services, chdir fix)?
  • PR needs release note in the newsfragments directory (you can look here for inspiration on how release note looks like).

@notdian
Copy link
Contributor Author

notdian commented Jan 28, 2025

the chdir and services/includes are related, you can't have one without the other, if you think otherwise, I can split them.

@p12tic
Copy link
Collaborator

p12tic commented Jan 28, 2025

the chdir and services/includes are related, you can't have one without the other, if you think otherwise, I can split them.

You're right, my head is not working.

@cfunkhouser
Copy link

I have confirmed that this fixes #1109 for my use cases.

@notdian
Copy link
Contributor Author

notdian commented Jan 29, 2025

@p12tic can we release this?

@p12tic p12tic force-pushed the file_not_found_error branch from 0a78aba to e03d675 Compare January 30, 2025 23:24
@p12tic
Copy link
Collaborator

p12tic commented Jan 30, 2025

Rebased.

@p12tic p12tic merged commit d1ba2f4 into containers:main Jan 30, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants