-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(cmd): flags override env vars #16280
Conversation
@@ -23,31 +22,20 @@ or execute a literal Flux query contained in a file by specifying the file prefi | |||
} | |||
|
|||
var queryFlags struct { | |||
OrgID string | |||
Org string | |||
organization |
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'm not wild about this (and some of the supporting methods); it's a bit harder to understand and feels like it's a separation of concerns violation (CLI and service logic starting to bleed into the organization struct).
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.
fair point, I tried to tread really lightly when I added this but crossed over boundaries a little. on the flip, this does DRY up a lot of cobra stuff across the board, and organization lookups by name all go through one code path here. I'm open to suggestions on where to put this as well
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 agree it's a little hard to understand at beginning. But overall, it would help us to make all of our flags more consistent. So still worth it in my opinion. Ideally, CLI should inherent all the behavior of the API, plus some interactive features and file parsing.
6fb1b5b
to
ebfcd6e
Compare
1db0a54
to
d776e17
Compare
d776e17
to
9fd0222
Compare
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.
LGTM, maybe clarify the org embedding thing with @jademcgough before you merge it.
@jademcgough let's figure out what to do with this PR. |
this was remedied remedied here: #16491 closing this issue as its completed now |
Closes #16246