Add support for setting environments secrets#3769
Conversation
| in the current directory, query the GitHub API, scan the user's `~/.ssh/config` file, clone or fetch git | ||
| repositories, etc. Naturally, none of these things should ever happen for real when running tests, unless | ||
| you are sure that any filesystem operations are stricly scoped to a location made for and maintained by the | ||
| you are sure that any filesystem operations are strictly scoped to a location made for and maintained by the |
There was a problem hiding this comment.
Unrelated, a small typo I noticed.
| return setRun(opts) | ||
| }, | ||
| } | ||
| cmd.Flags().StringVarP(&opts.OrgName, "org", "o", "", "List secrets for an organization") |
There was a problem hiding this comment.
I believe this description was wrong, fixed it while I was touching the code around.
pkg/cmd/secret/set/set.go
Outdated
| } else { | ||
| err = putRepoSecret(client, pk, baseRepo, opts.SecretName, encoded) | ||
| if envName != "" { |
There was a problem hiding this comment.
Not sure if it's the most idiomatic way of writing this, let me know if you prefer it as:
if orgName != "" {
err = putOrgSecret(client, host, pk, *opts, encoded)
} else if envName != "" {
err = putEnvSecret(client, pk, baseRepo, envName, opts.SecretName, encoded)
} else {
err = putRepoSecret(client, pk, baseRepo, opts.SecretName, encoded)
}There was a problem hiding this comment.
Collapsing that nested conditional nto the else as you suggested would be a welcome change; it's so minor I'll just push a commit before I merge.
| assert.Equal(t, payload.EncryptedValue, "UKYUCbHd0DJemxa3AOcZ6XcsBwALG9d4bpB8ZT0gSV39vl3BHiGSgj8zJapDxgB2BwqNqRhpjC4=") | ||
| } | ||
|
|
||
| func Test_setRun_env(t *testing.T) { |
There was a problem hiding this comment.
This is almost like Test_setRun_repo above, except the 2nd mock and the EnvName option... Might be better to parametrize the existing test?
There was a problem hiding this comment.
I think this is fine for now!
vilmibm
left a comment
There was a problem hiding this comment.
Thanks for this!! It's super helpful and I appreciate how neatly you fit it into our existing code/UX.
| assert.Equal(t, payload.EncryptedValue, "UKYUCbHd0DJemxa3AOcZ6XcsBwALG9d4bpB8ZT0gSV39vl3BHiGSgj8zJapDxgB2BwqNqRhpjC4=") | ||
| } | ||
|
|
||
| func Test_setRun_env(t *testing.T) { |
There was a problem hiding this comment.
I think this is fine for now!
pkg/cmd/secret/set/set.go
Outdated
| } else { | ||
| err = putRepoSecret(client, pk, baseRepo, opts.SecretName, encoded) | ||
| if envName != "" { |
There was a problem hiding this comment.
Collapsing that nested conditional nto the else as you suggested would be a welcome change; it's so minor I'll just push a commit before I merge.
Support to list environemnt secrets was added in #3270 but it would be nice to also be able to set them.
Fixes #3265