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

Skip build config section validation when passing no-build option #5780

Conversation

smasset
Copy link

@smasset smasset commented Mar 13, 2018

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.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ 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.

@smasset smasset force-pushed the skip-build-section-validation-with-no-build-option branch from d2e4ba7 to 906a587 Compare March 13, 2018 21:53
@smasset
Copy link
Author

smasset commented Apr 11, 2018

ping

@turbo
Copy link

turbo commented Apr 25, 2018

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.

@smasset
Copy link
Author

smasset commented Apr 27, 2018

@dnephin, @dsanders11, any advice ?

Copy link

@shin- shin- left a 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:
Copy link

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?

Copy link
Author

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.

@chmorgan
Copy link

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.

@ijc
Copy link

ijc commented May 20, 2019

Pretty busy this week. Will see if I can update the PR if I get another sleepless night.

@smasset are you still planing to update?

@glours
Copy link
Contributor

glours commented Jul 13, 2022

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.

@glours glours closed this Jul 13, 2022
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.

8 participants