Skeleton for dry-run under alpha command by glours · Pull Request #10173 · docker/compose (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation6 Commits6 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
What I did
Add a dry-run command skeleton under alpha command to test and incrementally implement --dry-run flag
Related issue
https://docker.atlassian.net/browse/ENV-1
start of work for #1203
(not mandatory) A picture of a cute animal, if possible in relation to what you did
Codecov Report
Base: 73.89% // Head: 73.89% // No change to project coverage 👍
Coverage data is based on head (c280ae2) compared to base (5a2b7b8).
Patch has no changes to coverable lines.
❗ Current head c280ae2 differs from pull request most recent head fb36f7f. Consider uploading reports for the commit fb36f7f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@ ## v2 #10173 +/- ##
Coverage 73.89% 73.89%
Files 2 2
Lines 272 272
Hits 201 201
Misses 60 60
Partials 11 11
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.
Comment on lines 71 to 82
| cli, err := command.NewDockerCli() |
|---|
| if err != nil { |
| return err |
| } |
| err = cli.Initialize(flags.NewClientOptions(), command.WithInitializeClient(func(cli *command.DockerCli) (client.APIClient, error) { |
| dryRunClient := api.NewDryRunClient() |
| dryRunClient.WithAPIClient(s.apiClient()) |
| return dryRunClient, nil |
| })) |
| if err != nil { |
| return err |
| } |
| s.dockerCli = cli |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could make it simpler by making API client an attribute in composeService, initialize with dockerCli.Client() and override here with NewDryRunClient.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok as we just discussed, I'll open a PR to remove the dockerCli attribute in composeService struct, this way we could be sure that the composeService.apiClient() will be used everywhere.
When merged, we'll be able to safely introduce a APIClient attribute to composeService and be sure we won't have multiple kind of APIClient calls (like service.dockerCli.client() used at many place in the source code)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok after digging a little bit, the dockerCli attribute is used widely by the CLI APIs, so it seams safer to override the APIClient of the existing command.Cli
Signed-off-by: Guillaume Lours 705411+glours@users.noreply.github.com
Signed-off-by: Guillaume Lours 705411+glours@users.noreply.github.com
update documentation
Signed-off-by: Guillaume Lours 705411+glours@users.noreply.github.com
Signed-off-by: Guillaume Lours 705411+glours@users.noreply.github.com
Signed-off-by: Guillaume Lours 705411+glours@users.noreply.github.com
Signed-off-by: Guillaume Lours 705411+glours@users.noreply.github.com
Reviewers
+1 more reviewer
Reviewers whose approvals may not affect merge requirements