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

Skeleton for dry-run under alpha command #10173

Merged
merged 6 commits into from Jan 20, 2023
Merged

Skeleton for dry-run under alpha command #10173

merged 6 commits into from Jan 20, 2023

Conversation

glours
Copy link
Contributor

@glours glours commented Jan 13, 2023

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
image

@codecov
Copy link

codecov bot commented Jan 13, 2023

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.

@glours glours marked this pull request as ready for review January 13, 2023 14:29
@glours glours requested review from a team, nicksieger, ndeloof, StefanScherer, ulyssessouza, milas and laurazard and removed request for a team January 13, 2023 14:29
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
Copy link
Contributor

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.

Copy link
Contributor Author

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)

Copy link
Contributor Author

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

pkg/api/dryrunclient.go Outdated Show resolved Hide resolved
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>
@glours glours mentioned this pull request Jan 18, 2023
@glours glours merged commit d5d9f67 into docker:v2 Jan 20, 2023
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

3 participants