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

Add --include-deps to push command #10044

Merged
merged 3 commits into from Dec 15, 2022
Merged

Conversation

gferon
Copy link
Contributor

@gferon gferon commented Dec 6, 2022

What I did

Add --include-deps to the push sub-command, similar to what pull can do. I believe the default behavior is a little bit weird since you can't explicitly push the built image of a service that has declared dependencies. That being said, this change will break backwards compatibility, so I feel like we could

Cute animal tax - a photo of my containerized cat:

PXL_20201120_110811709 PORTRAIT(2)

@ndeloof
Copy link
Contributor

ndeloof commented Dec 9, 2022

While I don't expect many users to rely on this, I'm a bit concerned about another backward compatibility break.
Maybe better introduce --no-deps to mimic same option used by up?
Also, to avoid code duplication, could define a func to disable all services but selected ones

@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Base: 76.98% // Head: 75.79% // Decreases project coverage by -1.19% ⚠️

Coverage data is based on head (91f7dcb) compared to base (8c39b5b).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10044      +/-   ##
==========================================
- Coverage   76.98%   75.79%   -1.20%     
==========================================
  Files           2        2              
  Lines         252      252              
==========================================
- Hits          194      191       -3     
- Misses         51       53       +2     
- Partials        7        8       +1     
Impacted Files Coverage Δ
pkg/e2e/framework.go 74.04% <0.00%> (-1.28%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gferon
Copy link
Contributor Author

gferon commented Dec 9, 2022

While I don't expect many users to rely on this, I'm a bit concerned about another backward compatibility break.

Fair point, I can definitely move it to a --no-deps flag instead, but this would be inconsistent with docker-compose pull. What about setting the value of this flag to true instead and let users pass --include-deps=false instead?

Also, to avoid code duplication, could define a func

Sure, can you point me to a good place to do this? I'm not familiar with this codebase to select a proper location myself.

@ndeloof
Copy link
Contributor

ndeloof commented Dec 9, 2022

I didn't noticed --include-deps flag existed for pull, makes more sense now. Both should be aligned. Minor compatibility break probably is acceptable from this point of view.

you can just extract https://github.com/docker/compose/blob/v2/cmd/compose/up.go#L57-L66 into a func inside up.go and call it from both command - by the way, can also use it for pull where code already is duplicated

@ndeloof
Copy link
Contributor

ndeloof commented Dec 9, 2022

also need to regenerate docs to reflect new flag using make docs

Signed-off-by: Gabriel Féron <g@leirbag.net>
@gferon
Copy link
Contributor Author

gferon commented Dec 9, 2022

@ndeloof not sure why you meant up.go, I extracted the function in pull.go - if you really wanted it to be in up.go I can change it.

Otherwise, this should be ready.

cmd/compose/pull.go Outdated Show resolved Hide resolved
Signed-off-by: Gabriel Féron <g@leirbag.net>
@ndeloof ndeloof merged commit 1cb5536 into docker:v2 Dec 15, 2022
@gferon gferon deleted the push-include-deps branch December 15, 2022 10:44
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.

None yet

2 participants