Allow env to be updated via specific key in resource #60040

philipgough · 2018-02-19T15:29:18Z

What this PR does / why we need it:

This change allows users of the oc client to specify a list of comma-separated keys when running oc set env which should be imported from a resource i.e configmap or secret

This can be useful when a number of applications want to share a configuration object but don't want to pollute a resource with unused environment

Release note:

Allow kubectl env to specify which keys to import from a config map

soltysh · 2018-02-19T15:33:27Z

/ok-to-test

soltysh

I'm also missing a test case for the new functionality.

soltysh · 2018-02-19T16:43:08Z

pkg/kubectl/cmd/set/set_env.go

 	if o.List && len(o.Output) > 0 {
 		return cmdutil.UsageErrorf(o.Cmd, "--list and --output may not be specified together")
 	}

+	if len(o.Keys) > 0 && len(o.From) < 1 {


len(o.From) == 0 is the preferred approach.

soltysh · 2018-02-19T16:44:34Z

pkg/kubectl/cmd/set/set_env.go

@@ -278,7 +293,15 @@ func (o *EnvOptions) RunEnv(f cmdutil.Factory) error {
 							},
 						},
 					}
-					env = append(env, envVar)
+					if len(o.Keys) > 0 {


Can you make a helper out of it, instead of copy&pasting it for secret here and config map below.

soltysh

/lgtm

soltysh · 2018-02-20T08:51:42Z

/retest

soltysh

/lgtm

soltysh · 2018-02-21T11:30:41Z

/assign @deads2k
for approval

deads2k · 2018-02-21T20:35:41Z

pkg/kubectl/cmd/set/set_env.go

-	if len(o.Filenames) == 0 && len(resources) < 1 {
-		return cmdutil.UsageErrorf(cmd, "one or more resources must be specified as <resource> <name> or <resource>/<name>")
+func (o *EnvOptions) addEnvVarToList(key string, env []v1.EnvVar, envVar v1.EnvVar) []v1.EnvVar {
+	if len(o.Keys) > 0 {


Seems weird to have the skip here. Why not skip down in the method iterating over keys? This feels like a function that sometimes disobeys me.

A contains(needle string, haystack []string) could hold this logic.

Originally had the code where we iterate over the keys, but was asked to move it to a helper since it was repeated for both configmap and secret

soltysh

/lgtm

deads2k · 2018-02-22T13:05:05Z

/approve

k8s-ci-robot · 2018-02-22T13:05:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, PhilipGough, soltysh

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:

~~pkg/kubectl/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-02-22T13:24:58Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-22T14:08:55Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

m1093782566 · 2018-02-23T07:44:58Z

The following test case can easily flakes, I am not clear about neither the root cause nor if this PR breaks it. Just FYI.

=== RUN   TestSetEnvRemote
--- FAIL: TestSetEnvRemote (0.09s)
        Error Trace:    set_env_test.go:523
                        fake.go:41
                        client.go:249
                        client.go:173
                        client.go:602
                        fake.go:119
                        request.go:664
                        request.go:736
                        helper.go:140
                        set_env.go:451
                        set_env_test.go:539
        Error:          "{"apiVersion":"extensions/v1beta1","spec":{"selector":null}}" does not contain ""value":"prod""
        Messages:       env not updated for &v1.Deployment{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:v1.ObjectMeta{Name:"nginx", GenerateName:"", Namespace:"", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*v1.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]v1.OwnerReference(nil), Initializers:(*v1.Initializers)(nil), Finalizers:[]string(nil), ClusterName:""}, Spec:v1.DeploymentSpec{Replicas:(*int32)(nil), Selector:(*v1.LabelSelector)(nil), Template:v1.PodTemplateSpec{ObjectMeta:v1.ObjectMeta{Name:"", GenerateName:"", Namespace:"", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*v1.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]v1.OwnerReference(nil), Initializers:(*v1.Initializers)(nil), Finalizers:[]string(nil), ClusterName:""}, Spec:v1.PodSpec{Volumes:[]v1.Volume(nil), InitContainers:[]v1.Container(nil), Containers:[]v1.Container{v1.Container{Name:"nginx", Image:"nginx", Command:[]string(nil), Args:[]string(nil), WorkingDir:"", Ports:[]v1.ContainerPort(nil), EnvFrom:[]v1.EnvFromSource(nil), Env:[]v1.EnvVar(nil), Resources:v1.ResourceRequirements{Limits:v1.ResourceList(nil), Requests:v1.ResourceList(nil)}, VolumeMounts:[]v1.VolumeMount(nil), VolumeDevices:[]v1.VolumeDevice(nil), LivenessProbe:(*v1.Probe)(nil), ReadinessProbe:(*v1.Probe)(nil), Lifecycle:(*v1.Lifecycle)(nil), TerminationMessagePath:"", TerminationMessagePolicy:"", ImagePullPolicy:"", SecurityContext:(*v1.SecurityContext)(nil), Stdin:false, StdinOnce:false, TTY:false}}, RestartPolicy:"", TerminationGracePeriodSeconds:(*int64)(nil), ActiveDeadlineSeconds:(*int64)(nil), DNSPolicy:"", NodeSelector:map[string]string(nil), ServiceAccountName:"", DeprecatedServiceAccount:"", AutomountServiceAccountToken:(*bool)(nil), NodeName:"", HostNetwork:false, HostPID:false, HostIPC:false, ShareProcessNamespace:(*bool)(nil), SecurityContext:(*v1.PodSecurityContext)(nil), ImagePullSecrets:[]v1.LocalObjectReference(nil), Hostname:"", Subdomain:"", Affinity:(*v1.Affinity)(nil), SchedulerName:"", Tolerations:[]v1.Toleration(nil), HostAliases:[]v1.HostAlias(nil), PriorityClassName:"", Priority:(*int32)(nil), DNSConfig:(*v1.PodDNSConfig)(nil)}}, Strategy:v1.DeploymentStrategy{Type:"", RollingUpdate:(*v1.RollingUpdateDeployment)(nil)}, MinReadySeconds:0, RevisionHistoryLimit:(*int32)(nil), Paused:false, ProgressDeadlineSeconds:(*int32)(nil)}, Status:v1.DeploymentStatus{ObservedGeneration:0, Replicas:0, UpdatedReplicas:0, ReadyReplicas:0, AvailableReplicas:0, UnavailableReplicas:0, Conditions:[]v1.DeploymentCondition(nil), CollisionCount:(*int32)(nil)}}
        Error Trace:    set_env_test.go:540
        Error:          Received unexpected error:
                        at least one environment variable must be provided

philipgough · 2018-02-23T10:24:22Z

@m1093782566 thanks will take a look asap

deads2k · 2018-02-23T13:19:30Z

The following test case can easily flakes, I am not clear about neither the root cause nor if this PR breaks it. Just FYI.

I just hit this locally. Seems consistent for me.

deads2k · 2018-02-23T13:25:33Z

opened #60302

soltysh · 2018-02-23T13:45:23Z

The fix is in #60303

k8s-ci-robot requested review from deads2k and derekwaynecarr February 19, 2018 15:29

soltysh self-assigned this Feb 19, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 19, 2018

philipgough force-pushed the keys-from-cm branch from ee1734f to 0a38254 Compare February 19, 2018 15:35

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 19, 2018

philipgough force-pushed the keys-from-cm branch from 0a38254 to 71f477f Compare February 19, 2018 15:42

soltysh added this to the v1.10 milestone Feb 19, 2018

soltysh requested changes Feb 19, 2018

View reviewed changes

soltysh mentioned this pull request Feb 19, 2018

Support update env from object for given keys openshift/origin#18575

Closed

philipgough force-pushed the keys-from-cm branch from 71f477f to 2a5a40b Compare February 19, 2018 22:53

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 19, 2018

soltysh approved these changes Feb 20, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2018

philipgough force-pushed the keys-from-cm branch from 2a5a40b to de5f330 Compare February 21, 2018 11:09

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2018

soltysh approved these changes Feb 21, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2018

k8s-ci-robot assigned deads2k Feb 21, 2018

philipgough force-pushed the keys-from-cm branch from de5f330 to 9a7bf3b Compare February 21, 2018 15:51

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2018

philipgough force-pushed the keys-from-cm branch from 9a7bf3b to 8dfe21d Compare February 21, 2018 16:45

deads2k reviewed Feb 21, 2018

View reviewed changes

Allow env to be updated via specific key in resource

905a069

philipgough force-pushed the keys-from-cm branch from 8dfe21d to 905a069 Compare February 22, 2018 11:01

soltysh approved these changes Feb 22, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2018

k8s-github-robot merged commit c85ede5 into kubernetes:master Feb 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow env to be updated via specific key in resource #60040

Allow env to be updated via specific key in resource #60040

philipgough commented Feb 19, 2018 •

edited

Loading

soltysh commented Feb 19, 2018

soltysh left a comment

soltysh Feb 19, 2018

soltysh Feb 19, 2018

soltysh left a comment

soltysh commented Feb 20, 2018

soltysh left a comment

soltysh commented Feb 21, 2018

deads2k Feb 21, 2018

philipgough Feb 22, 2018

soltysh left a comment

deads2k commented Feb 22, 2018

k8s-ci-robot commented Feb 22, 2018

k8s-github-robot commented Feb 22, 2018

k8s-github-robot commented Feb 22, 2018

m1093782566 commented Feb 23, 2018 •

edited

Loading

philipgough commented Feb 23, 2018

deads2k commented Feb 23, 2018

deads2k commented Feb 23, 2018

soltysh commented Feb 23, 2018

Allow env to be updated via specific key in resource #60040

Allow env to be updated via specific key in resource #60040

Conversation

philipgough commented Feb 19, 2018 • edited Loading

soltysh commented Feb 19, 2018

soltysh left a comment

Choose a reason for hiding this comment

soltysh Feb 19, 2018

Choose a reason for hiding this comment

soltysh Feb 19, 2018

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

soltysh commented Feb 20, 2018

soltysh left a comment

Choose a reason for hiding this comment

soltysh commented Feb 21, 2018

deads2k Feb 21, 2018

Choose a reason for hiding this comment

philipgough Feb 22, 2018

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

deads2k commented Feb 22, 2018

k8s-ci-robot commented Feb 22, 2018

k8s-github-robot commented Feb 22, 2018

k8s-github-robot commented Feb 22, 2018

m1093782566 commented Feb 23, 2018 • edited Loading

philipgough commented Feb 23, 2018

deads2k commented Feb 23, 2018

deads2k commented Feb 23, 2018

soltysh commented Feb 23, 2018

philipgough commented Feb 19, 2018 •

edited

Loading

m1093782566 commented Feb 23, 2018 •

edited

Loading