-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 env sources to secrets #241
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sethpollack If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this.
pkg/types/kustomization.go
Outdated
} | ||
|
||
// CommandSources contains some generic sources for secrets. | ||
// Only one field can be set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why one field? If you really need this mandatory, please add some code for checking this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, copied that from DataSources. I don't think that is actually the case, I'll remove from both.
pkg/types/kustomization.go
Outdated
// Map of keys to commands to generate the values | ||
Commands map[string]string `json:",commands,omitempty" yaml:",inline,omitempty"` | ||
Commands map[string]string `json:"commands,omitempty" yaml:"literals,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/literals/commands/
pkg/types/kustomization.go
Outdated
Commands map[string]string `json:"commands,omitempty" yaml:"literals,omitempty"` | ||
// EnvCommand to output lines of key=val pairs to create a secret. | ||
// i.e. a Docker .env file or a .ini file. | ||
EnvCommand string `json:"envCommand,omitempty" yaml:"env,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
envCommand
or env
? Let's keep them consistent.
@@ -30,7 +30,7 @@ func NewResMapFromSecretArgs( | |||
secretList []types.SecretArgs) (ResMap, error) { | |||
var allResources []*resource.Resource | |||
for _, args := range secretList { | |||
s, err := f.MakeSecret(args) | |||
s, err := f.MakeSecret(&args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to change the argument of MakeSecret
to a pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to keep it consistent with MakeConfigMap
, should I change that too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Let's keep it consistent with MakeConfigMap
.
var kvs []kvPair | ||
for k, cmd := range sources { | ||
content, err := f.createSecretKey(cmd) | ||
fmt.Println("createSecretKey:", content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a good idea to print the secret content. Can we remove this print?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, that was for debugging :)
return fmt.Errorf("%q is not a valid key name for a ConfigMap: %s", keyName, strings.Join(errs, ";")) | ||
} | ||
if _, entryExists := secret.Data[keyName]; entryExists { | ||
return fmt.Errorf("cannot add key %s, another key by that name already exists: %v", keyName, secret.Data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please not include secret.Data in the error message.
return s, nil | ||
} | ||
|
||
func addKvToSecret(secret *corev1.Secret, keyName, data string) error { | ||
// Note, the rules for SecretKeys keys are the exact same as the ones for ConfigMap. | ||
if errs := validation.IsConfigMapKey(keyName); len(errs) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for doing this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
can the docs/kustomization.yaml please be updated? |
fixes #74