-
Notifications
You must be signed in to change notification settings - Fork 0
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 --dry-run and --verbose flags #23
Conversation
if options.Reset { | ||
if options.Verbose { | ||
fmt.Fprintf(os.Stderr, "removing container first") | ||
} | ||
if err := docker.Remove(ctx, containerName); err != nil { |
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.
i just realized there is a bug here. What if we run with reset but the container doesn't exist? We don't want to fail. Not to do with the PR but just thought I would flag it. I probably wrote the bug.
if err := docker.Remove(ctx, containerName); err != nil { | ||
return fmt.Errorf("failed to remove container %q: %w", containerName, err) | ||
} | ||
} | ||
|
||
var runOptions []docker.RunOption |
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.
we can probably do away with creating an options slice since all options accept their values. we can pass the empty string to WithWorkDir because even if we dont't pass the option it probably initialises the backing option to empty string anyways.
if options.Verbose { | ||
fmt.Fprintln(os.Stderr) | ||
} | ||
if !options.DryRun { |
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.
Dry-run does not print the banner but still starts the docker process? Wouldn't the flag be --no-banner
?
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.
nvm
No description provided.