fix(influx): add env vars to cli usage and normalize usage and flag/env var priority #16491
Conversation
jsteenb2
force-pushed
the
16246/add_env_vars_to_cli_usage
branch
from
8f48640
to
7a98a48
Compare
|
@sanderson this will make a bunch of changes to the CLI help outputs. The env vars will be exposed so folks will know what they are when they see the |
jsteenb2
force-pushed
the
16246/add_env_vars_to_cli_usage
branch
2 times, most recently
from
078357e
to
11cd3c3
Compare
| } | ||
|
|
||
| if userName := authCreateFlags.user; userName != "" { | ||
| userSvc, err := newUserService() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| user, err := userSvc.FindUser(ctx, platform.UserFilter{ | ||
| user, err := userSvc.FindUser(context.Background(), platform.UserFilter{ |
There was a problem hiding this comment.
I'd love to see us use some context signaling to cancel on the typical signals. However, this one was always context.Background().
There was a problem hiding this comment.
I wish that were the case, but the context that was removed above waws just context.Background() 🤷♂. It just happened to be instantiated well before its use case.
| cmd.Flags().StringVarP(&authorizationFindFlags.id, "id", "i", "", "The authorization ID") | ||
|
|
||
| return cmd | ||
| } | ||
|
|
||
| func newAuthorizationService(f Flags) (platform.AuthorizationService, error) { | ||
| func newAuthorizationService() (platform.AuthorizationService, error) { |
There was a problem hiding this comment.
I am never a big fan of anything global. If passing the flags bothers you, you could compose a struct and make this function a method.
There was a problem hiding this comment.
totally agree. I'd like to see all the commands use the builder pattern of sorts so we can dep inject. We'r enot quite there yet. That'll take some time.
| return //return here, since cobra already handles the error | ||
| } | ||
| c.Printf("See '%s -h' for help\n", c.CommandPath()) | ||
| var flags struct { |
There was a problem hiding this comment.
i don't really like such a global thing.
There was a problem hiding this comment.
totally agree. I didn't remedy all the issues in this pkg on this go round. However, I agree, the global constructs in this are a major issue and should be fixed. I didn't do it here b/c it was already bigger body of work than I anticipated 😢
jsteenb2
force-pushed
the
16246/add_env_vars_to_cli_usage
branch
4 times, most recently
from
10aa46d
to
a199998
Compare
…nv var priority also rids us of the cobra tutorial code and encapsulates things into funcs
jsteenb2
force-pushed
the
16246/add_env_vars_to_cli_usage
branch
from
a199998
to
ee3198f
Compare
also rids the pkg of the cobra tutorial init style and encapsulates things into funcs. One step closer to builder pattern for this cmd pkg.
Closes #16246
Closes #16048