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

Add apply command for creds and params #1715

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

carolynvs
Copy link
Member

@carolynvs carolynvs commented Aug 11, 2021

What does this change

  • Add a new command for credential and parameter sets which allow you toapply changes (or create net new) from a file.
  • Fixes how we resolve params and credentials to allow referencing a global cs/ps from within a namespace during install.

What issue does it fix

This is part of #1703. Next up is for installations which is a bit more complex.

Notes for the reviewer

N/A

Checklist

  • Unit Tests
  • Documentation
  • Schema (porter.yaml)

@carolynvs carolynvs force-pushed the apply-cs-ps-sets branch 4 times, most recently from c95bc65 to 1a09e3e Compare August 11, 2021 20:10
Add a new command for credential and parameter sets which allow you to
apply changes (or create net new) from a file.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@carolynvs carolynvs marked this pull request as ready for review August 11, 2021 21:20
porter parameters generate myparams --reference SOME_BUNDLE
porter parameters show myparams --output yaml > myparams.yaml
`,
Example: ` porter parameters apply --file myparams.yaml`,
Copy link
Member

Choose a reason for hiding this comment

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

Should be porter parameters apply myparams.yaml, right? If so, same update needed for creds.

This is a good time to mention: thoughts on using a -f/--file flag to pass in filename vs positional arg? I'm sure you'd thought of it. Endorsement for the former is that it'd be natural for users of kubectl -- but that doesn't nec. mean it's the right way to go for Porter. No strong opinion here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had strongly considered the -f flag (as you can see since I forgot to fix that example). I like to use a positional argument when the argument is required, is always specified, and is the object of the (command) sentence.

The file parameter fits all those characteristics and that is how the other commands in porter act as well (such as install). Having file be a positional argument is more consistent with porter itself, which I feel is more important that being consistent with kubectl.

Also there aren't a lot of other arguments that would make someone likely to be unsure what the positional argument should be. At most you are specifying porter installation apply FILE --namespace and to me namespace falls into the same category as say debug, or output. It's a general modifier for a bunch of commands, but it isn't the focus of the command, and therefore is unlikely to be accidentally specified as the positional argument.

If we think that people will instinctively type -f FILE from muscle memory, we can do what we do for the help text (which is supported through --help and a help subcommand). We could add a hidden file flag and bind it, so that either way works, but we document the way that matches the rest of porter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think that I should add that hidden flag?

Copy link
Member

Choose a reason for hiding this comment

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

Your call! I'd say we proceed w/o but if it's trivially easy to add... maybe so :)

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@carolynvs carolynvs merged commit f801cc8 into getporter:release/v1 Aug 13, 2021
@carolynvs carolynvs deleted the apply-cs-ps-sets branch August 13, 2021 13:27
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.

2 participants