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

Change configuration verbs for getting and setting values #563

Merged
merged 1 commit into from
Nov 16, 2017
Merged

Conversation

ellismg
Copy link
Contributor

@ellismg ellismg commented Nov 14, 2017

Move from two top level commands text and secret to a single set
command to set a value. To store a value encrypted, use --secret as
an argument to set.

Renames ls to get. The command is otherwise unchanged, so get
with no additional arguments will print all the configuration values
for a stack (in a table format), while get <key-name> will print the
value for that key by itself (useful for scripting).

Fixes #552

@lindydonna
Copy link

I'm worried that having --secret not be the default will cause people to check in secrets. Is it too verbose to add a log message like "Added key {key} with value {value} as plaintext"?

I didn't understand the change from ls to get, could you clarify the usage?

@ellismg
Copy link
Contributor Author

ellismg commented Nov 14, 2017

I didn't understand the change from ls to get, could you clarify the usage?

Sure. So get operates in two modes, depending on if an argument is passed or not. If you don't pass an argument, we present a nice table of configuration values:

[matell@matell build-failure-notifier]$ pulumi config get
KEY                              VALUE                           
aws:config:region                us-west-2                       
slack-token                      ********                        
watched-branches                 master,production,staging       

(Note that in the above example, I could have passed --show-secrets to have the output display the secret value of my slack token). When run with a single argument, we just display the value:

[matell@matell build-failure-notifier]$ pulumi config get aws:config:region
us-west-2

We actually this feature ourselves in some scripts: https://github.com/pulumi/pulumi-service/blob/master/scripts/describe-stack.sh#L27

So the ls verb makes sense for the first case, but using ls with an argument as a way to get at the configuration value was pretty confusing, so the the plan is to just move to get which works all right as either "get me everything" or "get me this specific value". It also has some nice symmetry with set now that we are using that.

Copy link

@lindydonna lindydonna 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. One suggestion:

I'm worried that having --secret not be the default will cause people to check in secrets. Is it too verbose to add a log message like "Added key {key} with value {value} as plaintext"?

@ellismg
Copy link
Contributor Author

ellismg commented Nov 14, 2017

I'm worried that having --secret not be the default will cause people to check in secrets. Is it too verbose to add a log message like "Added key {key} with value {value} as plaintext"?

Happy to do that, @joeduffy what do you think? Do you think this would get annoying after a bit? I definitely thing it would help train people to do the right thing. Do you think we'd want some sort of message for the --secret case @lindydonna ?

@lindydonna
Copy link

@ellismg Good point. To be consistent, perhaps Added key {value} with encrypted value.

@ellismg
Copy link
Contributor Author

ellismg commented Nov 15, 2017

Thanks, @lindydonna I added another commit (and used the word "set" instead of "added" in the message, since it matches the verb and also you can use pulumi config set to overwrite an existing value).

@joeduffy
Copy link
Member

I tend not to love "simple" commands printing messages upon success, as the general UNIX philosophy is "no output means success." But it's fine to violate this in specific places with real use cases, and this certainly seems like a fine one (security).

Note that it may be interesting eventually to run something like this: https://github.com/awslabs/git-secrets. The idea would be we could reject attempts to add a secret-like value

error: the plaintext config value looks like a secret;
       pass --force to add it anyway, or --secret to encrypt

I realize it may be a bridge too far for now, but things like this can really improve our appeal to Enterprise developers, and is keeping with our theme of falling into the pit of success. Secret management is one of those areas that people just don't get right. It's not logical, but if this somehow just got added without me looking, I'd be a happy dude :-)

@joeduffy
Copy link
Member

Matt, how do I list all config values in the current stack? TBH, I really liked the old model of just saying pulumi config and having that be the default. Is that how it works again?

@joeduffy
Copy link
Member

Does this also change the default for --save to save for all stacks?

@lindydonna
Copy link

@joeduffy I had a similar idea. I created #570. We can just put it on the backlog.

@ellismg
Copy link
Contributor Author

ellismg commented Nov 15, 2017

Matt, how do I list all config values in the current stack? TBH, I really liked the old model of just saying pulumi config and having that be the default. Is that how it works again?

No, it doesn't work like that. You write pulumi config get (the lack of a specific key means print all keys).

Does this also change the default for --save to save for all stacks?

No, I had forgotten about that issue (since I think it was in my slack scrollback vs an issue) but I can change that. Do you want to keep the version without the --save to be stack specific (as is the behavior now and how pulumi worked when I joined) or do you want it to behave the same way? I can see arguments on both sides (I believe @lukehoban was not a fan of configuration applying to every stack by default, but I may be remembering wrong, since the theory was this locally saved configuration was very likely to be specific to a stack).

@joeduffy
Copy link
Member

joeduffy commented Nov 15, 2017

No, it doesn't work like that.

Curious what others think ... the way I wrote it is the way I'd prefer ... (get implies you're getting something specific, especially due to the symmetry with set).

I also think "show me the current config" is going to be a very common "show me stuff without making me think" operation (e.g., something's going wrong, dump the config). Making that easy to type and making that the thing people default to typing seems like a good idea.

not a fan of configuration applying to every stack by default

Right, but we don't apply it to every stack by default. I'm just saying --save persists to the config file. In my experience using this for customer workloads, the common workflow is

  • Iterate lots by setting private configs, testing, etc. for a single stack
  • Now I want to persist to share a setting with my team -- in my experience thus far, this is almost always global to all stacks

Unfortunately, because --save makes it specific to a stack, I almost always do the wrong thing with the second ... and then I'm left trying to figure out how to undo what I just did -- usually amongst lots of other edits I have in my worktree -- and redo it the right way. Kind of a PITA.

@ellismg
Copy link
Contributor Author

ellismg commented Nov 16, 2017

Based on the above discussion, I change things slightly. The text of the commit message is an accurate reflection of the state of the world:

 - `pulumi config ls` has been removed. Now, `pulumi config` with no
   arguments prints the table of configuration values for a stack and
   a new command `pulumi config get <key>` prints the value for a
   single configuration key (useful for scripting).
 - `pulumi config text` and `pulumi config secret` have been merged
   into a single command `pulumi config set`. The flag `--secret` can
   be used to encrypt the value we store (like `pulumi config secret`
   used to do).
 - To make it obvious that setting a value with `pulumi config set` is
   in plan text, we now echo a message back to the user saying we
   added the configuration value in plaintext.

I did not do the thing @joeduffy mentioned about having --save imply --all since as I tried to write the help text I couldn't figure out how to explain what was going on. So we continue with the model that by default configuration values are only set for the current stack and if you want to set them of all stacks you have to write --all. If we continue to find that when folks do --save they usually want to target all stacks, we can re-evaluate.

@joeduffy
Copy link
Member

LGTM 👍

A handful of UX improvments for config:

 - `pulumi config ls` has been removed. Now, `pulumi config` with no
   arguments prints the table of configuration values for a stack and
   a new command `pulumi config get <key>` prints the value for a
   single configuration key (useful for scripting).
 - `pulumi config text` and `pulumi config secret` have been merged
   into a single command `pulumi config set`. The flag `--secret` can
   be used to encrypt the value we store (like `pulumi config secret`
   used to do).
 - To make it obvious that setting a value with `pulumi config set` is
   in plan text, we now echo a message back to the user saying we
   added the configuration value in plaintext.

Fixes #552
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.

None yet

3 participants