add env sources to secrets #241
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[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 |
k8s-ci-robot
added
the
size/L
Liujingfang1
requested review from
monopole
and removed request for
grodrigues3
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.
Why one field? If you really need this mandatory, please add some code for checking this.
There was a problem hiding this comment.
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"` |
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.
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.
No need to change the argument of MakeSecret to a pointer.
There was a problem hiding this comment.
I was trying to keep it consistent with MakeConfigMap, should I change that too?
There was a problem hiding this comment.
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.
It's not a good idea to print the secret content. Can we remove this print?
There was a problem hiding this comment.
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.
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 { |
k8s-ci-robot
added
the
lgtm
|
can the docs/kustomization.yaml please be updated? |
fixes #74