-
Notifications
You must be signed in to change notification settings - Fork 16
feat: support env var as default flag value #75
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
base: main
Are you sure you want to change the base?
Conversation
| // THEN | ||
| compareToBaseline(this, StringArrayBaselineFormat, lines); | ||
| }); | ||
|
|
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.
Can we also add a redact: true test?
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.
Ah good catch, didn't have a case for redact in the formatting baselines.
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.
Oh at the moment there isn't any difference in the help text when redact: true is enabled. Should there be?
[--requiredBoolean/--noRequiredBoolean] required boolean flag [default = <from env REQUIRED_BOOLEAN>]
-h --help Print help information and exit
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.
At the moment the help text doesn't display the value of the environment variable, only the name. While I think it would be incredibly handy to print the value (or redacted value) we also still need to print the name and all of that takes up a lot of space. Not sure what the best display would be yet.
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.
Which of these options would be the best for displaying the default value? Right now we have the last one, which has the added benefit of not needing to read from the process to build the help text.
[--flag] some flag [default = ACTUAL_VALUE]
[--flag] some flag [default = ACTUAL_VALUE <from env REQUIRED_BOOLEAN>]
[--flag] some flag [default = <from env REQUIRED_BOOLEAN>]
With the following for redact: true
[--flag] some flag [default = ************]
[--flag] some flag [default = ************ <from env REQUIRED_BOOLEAN>]
[--flag] some flag [default = <from env REQUIRED_BOOLEAN>]
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.
My preference would be option (2) followed by (1) if length is a concern.
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.
Sounds good, we can do 2 and still have the backout to 3 if there is no value set.
3d56824 to
9640f8b
Compare
mkubilayk
left a comment
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.
My preference on formatting: #75 (comment).
|
The output now looks like this depending on value/config:
I switched the redact character from * to █ so that it was even more obvious that the value is purposefully hidden. |
3a7518d to
1c0caa8
Compare
Signed-off-by: mmolisani <[email protected]>
1c0caa8 to
af6de52
Compare
|
One problem with extending "default" to include environment variable values is that with normal defaults there is always a value but in the case where the variable is undefined then there is no value. Does it make sense to keep it as an extension of "default" or to split it off so that we can read from the env var and also have a default value? |
Resolves #62
Describe your changes
Expose functionality for application developers to pull the default value for a flag from an environment variable. This was already possible manually, but integrating it as a first-class feature allows for better formatting of error messages and the ability to "redact" the value in Stricli-generated output (for secrets).
Testing performed
Added several new test cases and updated the formatting baselines to achieve complete coverage.
Additional context
One side-effect is that
CommandContext["process"]is nowStricliProcessrather than justWritableStreamsto allowenvto be defined. The differences between the two types are just optional properties so there shouldn't be any issues.