fix(cli): --app flag can now be overridden by users #1399
Conversation
| type GlobalOpts struct { | ||
| appName string | ||
| prompt prompter | ||
| } |
There was a problem hiding this comment.
Curious if it's possible to keep GlobalOpts and therefore maintain DRYness across all the command Opts but just get rid of the bindAppName logic?
There was a problem hiding this comment.
GlobalOpts itself is not great because it resulted in commands getting a prompt whether they needed it or not, and we had a bunch of commands that never used it :(
An alternative that doesn't force the re-declaration of the --app flag in these sub-commands that I can think of is this: https://gist.github.com/efekarakus/37144a1a4b8792508c39e9927a020243
The benefit is that we don't have to remember to add the flag for all these sub-commands.
But, the downside is that I find this harder to understand as a reader than:
cmd.Flags().StringVarP(&vars.appName, appFlag, appFlagShort, tryReadingAppName(), appFlagDescription)
This makes it obvious if the --app flag is part of the command or not.
Yea after 2h44mins success :) |
efekarakus
added
do-not-merge
Resolves aws#1329 Removes GlobalOpts struct and the viper lib dependency from the CLI. Previously, if we ran "env init --app other" the overridden value "other" was ignored and only the workspace's app was used. This is an issue with viper as was found in aws#1329. Instead, with this change we get rid of Global Flags and force commands themselves to write whether they need an --app flag or not.
efekarakus
force-pushed
the
fix/global-app-flag
branch
from
355633a to
6938fb3
Compare
| return nil | ||
| } | ||
|
|
||
| func (o *initOpts) loadSvc() error { | ||
| if initSvcOpts, ok := o.initSvcCmd.(*initSvcOpts); ok { |
There was a problem hiding this comment.
Is there a way to test this case?
Resolves aws#1329 Removes GlobalOpts struct and the viper lib dependency from the CLI. Previously, if we ran "env init --app other" the overridden value "other" was ignored and only the workspace's app was used. This is an issue with viper as was found in aws#1329. Instead, with this change we get rid of Global Flags and force commands themselves to write whether they need an --app flag or not. _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
Resolves #1329
Removes GlobalOpts struct and the viper lib dependency from the CLI.
Previously, if we ran "env init --app other" the overridden value
"other" was ignored and only the workspace's app was used.
This is an issue with viper as was found in #1329. Instead, with this
change we get rid of Global Flags and force commands themselves to write
whether they need an --app flag or not.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.