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

handle accountKey arg with user and secret #500

Merged
merged 6 commits into from
Dec 10, 2024
Merged

handle accountKey arg with user and secret #500

merged 6 commits into from
Dec 10, 2024

Conversation

wildemat
Copy link
Contributor

@wildemat wildemat commented Dec 10, 2024

Ticket(s): FE-6231

Problem

  • A user can specify user, secret, and account-key arguments together without any messaging as to what is happening. This isn't clear on what is being used.

Solution

  • The use-case for a user to specify an account key would be to still leverage just-in-time database secrets, but skip the login step. Users could mint an account key via the UI and put it in an environment variable on their machine and have a completely headless CLI experience.
    • Don't allow secret and account-key. Make it clear that if they specify an account key, they are buying in to using the automatic short term database secrets we generate.
    • Allow user and account-key, but provide a debug log that we will ignore user. user always gets a value of at least default, so we don't want to yell about that.
    • Improve account-key description

Result

  • account key usage is more clear

Testing

  • updated expected error message

@wildemat wildemat requested a review from a team as a code owner December 10, 2024 18:12
@wildemat wildemat requested a review from jrodewig December 10, 2024 18:16
keySource = "--secret";
keySource = "user";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is this describing that the source of the key is a user-provided value? Not that specifically the --user arg was provided, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!


if (argv.user && argv.accountKey) {
logger.debug(
"Both 'user' and 'account-key' arguments were specified. 'account-key' will be used to mint database secrets. 'user' will be ignored.",
Copy link
Contributor

@jrodewig jrodewig Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use --accountKey in the help, but it looks like we accept both. I'll update the docs, but it may be good if our help hints output both or just the kebab-case version if that's preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put it back to camelcase for simplicity

@wildemat wildemat merged commit 42d02d6 into v3 Dec 10, 2024
4 checks passed
@wildemat wildemat deleted the fe-6231 branch December 10, 2024 21:35
@wildemat wildemat mentioned this pull request Dec 10, 2024
@cleve-fauna cleve-fauna mentioned this pull request Dec 13, 2024
@wildemat wildemat mentioned this pull request Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants