-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Skip build config section validation when passing no-build option #5780
Skip build config section validation when passing no-build option #5780
Conversation
Please sign your commits following these rules: $ git clone -b "skip-build-section-validation-with-no-build-option" [email protected]:smasset/compose.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f Amending updates the existing PR. You DO NOT need to open a new one. |
d2e4ba7
to
906a587
Compare
ping |
Need this. Because docker compose with multiple docker compose files still fails even when a build is overwritten with an image, when the build path doesn't exist. There is no need to validate build paths when they are overwritten. I guess this PR provides a workaround. |
@dnephin, @dsanders11, any advice ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay!
I'm still on the fence regarding allowing a configuration to be invalid, although I see the use-case.
That aside, I have a question on the design.
@@ -718,7 +732,7 @@ def process_service(service_config): | |||
for path in to_list(service_dict['env_file']) | |||
] | |||
|
|||
if 'build' in service_dict: | |||
if 'build' in service_dict and 'build' not in skipped_sections: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the design for the feature is made in a generic fashion, but we end up just making a couple string matches (here and in validate_paths
) which seems to defeat the purpose. What's the rationale for not removing the declared skipped_sections
from the config entirely? Wouldn't it make #1973 simpler to implement down the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review.
Not much of a rationale except I didn't think about it and minimum entropy production principle...
Now that you mention it, I'm mostly fine with it except I'm not sure if we should change original config that downward components might need.
Pretty busy this week. Will see if I can update the PR if I get another sleepless night.
Any updates on these changes? I'm trying to use a docker-compose.yml to bring services up after loading images and bumping into this issue as the build files are not present. |
@smasset are you still planing to update? |
Signed-off-by: Sebastien Masset <[email protected]>
906a587
to
6f9d32a
Compare
Thanks for taking the time to create this issue/pull request! Unfortunately, Docker Compose V1 has reached end-of-life and we are not accepting any more changes (except for security issues). Please try and reproduce your issue with Compose V2 or rewrite your pull request to be based on the v2 branch and create a new issue or PR with the relevant Compose V2 information. |
Partial fix for #1973 (covers only #3391 part).
Wasn't sure where and how to test the CLI part (didn't find existing tests for it). But will gladly add them with some guidance.