Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

saiaunghlyanhtet
Copy link

@saiaunghlyanhtet saiaunghlyanhtet commented Nov 30, 2024

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

Copy link

hashicorp-cla-app bot commented Nov 30, 2024

CLA assistant check
All committers have signed the CLA.

@saiaunghlyanhtet saiaunghlyanhtet marked this pull request as ready for review November 30, 2024 06:23
@saiaunghlyanhtet saiaunghlyanhtet requested a review from a team as a code owner November 30, 2024 06:23
@saiaunghlyanhtet saiaunghlyanhtet requested review from ltcarbonell and removed request for a team November 30, 2024 06:23
@saiaunghlyanhtet saiaunghlyanhtet force-pushed the feature/valut-token-renew-fail branch from 6944989 to 0bfa0c1 Compare December 2, 2024 06:49
Copy link
Contributor

@bosouza bosouza left a 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.

command/token_renew.go Outdated Show resolved Hide resolved
command/token_renew.go Outdated Show resolved Hide resolved
command/token_renew.go Outdated Show resolved Hide resolved
changelog/29060.txt Outdated Show resolved Hide resolved
@saiaunghlyanhtet saiaunghlyanhtet force-pushed the feature/valut-token-renew-fail branch 2 times, most recently from f9a4096 to 06a493c Compare January 1, 2025 13:11
@saiaunghlyanhtet saiaunghlyanhtet requested a review from a team as a code owner January 1, 2025 13:11
@saiaunghlyanhtet saiaunghlyanhtet force-pushed the feature/valut-token-renew-fail branch from 06a493c to 94a436b Compare January 1, 2025 13:14
@@ -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.
Copy link
Contributor

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.
Copy link
Contributor

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.

website/content/docs/commands/token/renew.mdx Outdated Show resolved Hide resolved
command/token_renew_test.go Outdated Show resolved Hide resolved
@saiaunghlyanhtet saiaunghlyanhtet changed the title CLI: add an option for renew command fail on non-fullfillable request… CLI: add an option for renew command fail on non-fulfillable request… Jan 5, 2025
command/token_renew.go Outdated Show resolved Hide resolved
@saiaunghlyanhtet saiaunghlyanhtet force-pushed the feature/valut-token-renew-fail branch from 94a436b to 028f058 Compare January 11, 2025 10:47
Copy link
Contributor

@bosouza bosouza left a 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>
@saiaunghlyanhtet saiaunghlyanhtet force-pushed the feature/valut-token-renew-fail branch from 028f058 to 5a7c32c Compare January 28, 2025 09:29
@saiaunghlyanhtet
Copy link
Author

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

Resolved conflict.

Copy link
Contributor

@bosouza bosouza left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants