-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
CLI: add an option for renew command fail on non-fulfillable request… #29060
base: main
Are you sure you want to change the base?
CLI: add an option for renew command fail on non-fulfillable request… #29060
Conversation
6944989
to
0bfa0c1
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.
Hi @saiaunghlyanhtet, thank you for the contribution! I'm currently gathering some opinions regarding this CLI-only implementation, will get back to you end of next week at the latest on that.
f9a4096
to
06a493c
Compare
06a493c
to
94a436b
Compare
changelog/29060.txt
Outdated
@@ -0,0 +1,3 @@ | |||
```release-note:improvement | |||
CLI: adds an optional flag (--fail-if-not-fullfilled) to the renew command, which lets the renew command fail on unfulfillable requests and allows command chaining to allow further executions. |
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.
there's one remaining typo in this fulfilled
. Also in the PR title and description.
@@ -53,3 +59,7 @@ flags](/vault/docs/commands) included on all commands. | |||
Vault will not honor this request for periodic tokens. If not supplied, Vault will use | |||
the default TTL. This is specified as a numeric string with suffix like "30s" | |||
or "5m". This is aliased as "-i". | |||
|
|||
- `--fail-if-not-fulfilled` - Allow command chaining after renew request fail. |
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 believe this should match the command Usage
field that's displayed in vault token renew --help
. This seems to be a consistent pattern across options for many (all?) commands. I'd suggest setting both Usage
and these docs to an initial short description of what the flag does, like your Fail if the requested TTL increment cannot be fully fulfilled.
and then move on with these additional details of how it can be used.
Also a nit: the current text seems a bit repetitive by mentioning that the flag allows command chaining twice.
94a436b
to
028f058
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.
Hey, thanks for the changes and sorry about the delay, I was out of office. This looks good, just take a look at the merge conflict and I'll approve and merge this
… to allow command chaining Signed-off-by: saiaunghlyanhtet <saiaunghlyanhtet2003@gmail.com>
028f058
to
5a7c32c
Compare
Resolved conflict. |
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.
Thank you for the contribution! I'll ping someone from the education team to review the docs and we should be good!
Description
What does this PR do?
This pull request adds an optional flag (--fail-if-not-fulfilled) to the renew command, which lets the renew command fail on unfulfillable requests and allows command chaining to allow further executions.
Related Issue: #28710