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

command: discard output from flags package and return errs directly #22373

Merged
merged 1 commit into from
Aug 16, 2019

Conversation

mildwonkey
Copy link
Contributor

Any command using meta.defaultFlagSet might occassionally exit before
the flag package's output got written. This caused flag error messages
to get lost. This PR discards the flag package output in favor of
directly returning the error to the end user.

This PR DIRECTLY CONFLICTS with #22372
I am equally happy to choose one over the other - if we are concerned about the implications of this change, starting with a single command is not a bad option.

Any command using meta.defaultFlagSet *might* occassionally exit before
the flag package's output got written. This caused flag error messages
to get lost. This PR discards the flag package output in favor of
directly returning the error to the end user.
@mildwonkey mildwonkey requested a review from a team August 7, 2019 15:21
Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question but non-blocking

@@ -353,26 +351,7 @@ func (m *Meta) contextOpts() *terraform.ContextOpts {
// defaultFlagSet creates a default flag set for commands.
func (m *Meta) defaultFlagSet(n string) *flag.FlagSet {
f := flag.NewFlagSet(n, flag.ContinueOnError)

// Create an io.Writer that writes to our Ui properly for errors.
// This is kind of a hack, but it does the job. Basically: create
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 deletion of code that says This is kind of a hack

@@ -36,7 +36,8 @@ func (c *StateMvCommand) Run(args []string) int {
cmdFlags.StringVar(&c.statePath, "state", "", "path")
cmdFlags.StringVar(&statePathOut, "state-out", "", "path")
if err := cmdFlags.Parse(args); err != nil {
return cli.RunResultHelp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder what RunResultHelp was (and if it is useful, or if it remains useful?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, it took me awhile to track this down.

This is an exit code from "github.com/mitchellh/cli". When it's used as an exit code, the cli package prints the usage text before exiting. It's used in other tf commands, but in this case (because cmdFlags.Parse also prints the usage) it resulted in the help/usage text getting printed twice.

I doubly appreciate the question because when I went back to check something I found the last bit of ~magic wherein the flags package was printing the usage, thanks! 👏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the info, and glad it helped! 🙌

@mildwonkey mildwonkey merged commit c9d62bb into master Aug 16, 2019
@mildwonkey mildwonkey deleted the mildwonkey/command-flag-errs branch August 16, 2019 12:31
@ghost
Copy link

ghost commented Sep 16, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Sep 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants