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

Allow env from resource with keys & updated tests #60636

Merged
merged 1 commit into from May 23, 2018

Conversation

@PhilipGough
Copy link
Contributor

PhilipGough commented Mar 1, 2018

What this PR does / why we need it:
This change allows users to pull environment from specific keys in secrets and configmaps using the kubectl set env command. User can provide a list of comma-separated keys with the --keys flag.

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

Improves test coverage of set env command

Release note:

Allow kubectl set env to specify which keys to import from a resource
return false
}

func (o *EnvOptions) getConfigResources(f cmdutil.Factory, cmd *cobra.Command) ([]*v1.ConfigMap, []*v1.Secret, error) {

This comment has been minimized.

@PhilipGough

PhilipGough Mar 1, 2018

Author Contributor

Needed to extract this code to allow it to be tested properly as I couldn't using the same mock client

@PhilipGough

This comment has been minimized.

Copy link
Contributor Author

PhilipGough commented Mar 1, 2018

/assign @soltysh

@jdumars

This comment has been minimized.

Copy link
Member

jdumars commented Mar 2, 2018

/ok-to-test
Please don't forget to sort out your release note. Thanks!

@PhilipGough

This comment has been minimized.

Copy link
Contributor Author

PhilipGough commented Mar 2, 2018

/test pull-kubernetes-e2e-gce

}
b := f.NewBuilder().
Internal().

This comment has been minimized.

@juanvallejo

juanvallejo Mar 6, 2018

Member

Do we need to deal with internal objects at all?
I see we end up converting down below anyway as we iterate through infos

This comment has been minimized.

@PhilipGough

PhilipGough Mar 7, 2018

Author Contributor

Not sure on this? Will I remove it?

This comment has been minimized.

@juanvallejo

juanvallejo Mar 7, 2018

Member

Updating it to Unstructured() should work

case *v1.ConfigMap:
configMaps = append(configMaps, from)
default:
return nil, nil, fmt.Errorf("unsupported resource specified in --from")

This comment has been minimized.

@juanvallejo

juanvallejo Mar 6, 2018

Member

maybe it'd help a user if we included which resources are supported in this error

return false
}

func (o *EnvOptions) getConfigResources(f cmdutil.Factory, cmd *cobra.Command) ([]*v1.ConfigMap, []*v1.Secret, error) {

This comment has been minimized.

@juanvallejo

juanvallejo Mar 6, 2018

Member

Do we plan on this being an ever-expanding list of resources that we support getting env values from?
If not, just rename this to something like getConfimapsAndSecrets or something similar.

If we do plan on growing the list of supported resources, I'd rather we return []runtime.Object. It would still only contain the resources of kinds that are supported, but the caller of this method could sort those out. @soltysh for an additional opinion on this.

This comment has been minimized.

@PhilipGough

PhilipGough Mar 7, 2018

Author Contributor

Given your comment below about aggregating on the struct I guess I can just change this function to a setter and return an error only, wdyt?

This comment has been minimized.

@juanvallejo

juanvallejo Mar 7, 2018

Member

Sure, please move the call to this method to Complete also. Thanks

return cmdutil.UsageErrorf(o.Cmd, "when specifying --keys, a configmap or secret must be provided with --from")
}

o.EnvArgs = envArgs

This comment has been minimized.

@juanvallejo

juanvallejo Mar 6, 2018

Member

why is this not being done in Complete?

This comment has been minimized.

@PhilipGough

PhilipGough Mar 7, 2018

Author Contributor

I can move that back up, I just thought it made more sense here since parsing the values might return an error?

This comment has been minimized.

@juanvallejo

juanvallejo Mar 7, 2018

Member

I would rather handle the error in Complete, in order to keep completion logic out of Validate

This comment has been minimized.

@PhilipGough

PhilipGough Mar 7, 2018

Author Contributor

Yeah that makes sense to me now I'll revert

return nil
}

// RunEnv contains all the necessary functionality for the OpenShift cli env command
func (o *EnvOptions) RunEnv(f cmdutil.Factory) error {
func (o *EnvOptions) RunEnv(f cmdutil.Factory, configMaps []*v1.ConfigMap, secrets []*v1.Secret) error {

This comment has been minimized.

@juanvallejo

juanvallejo Mar 6, 2018

Member

These should not be args passed to RunEnv. Aggregate them on the options struct

This comment has been minimized.

@juanvallejo

juanvallejo Mar 6, 2018

Member

We also should not be passing the factory as an arg here, but since that is pre-existing logic, can be addressed in a separate PR.

This comment has been minimized.

@PhilipGough

PhilipGough Mar 7, 2018

Author Contributor

"We also should not be passing the factory as an arg here, but since that is pre-existing logic, can be addressed in a separate PR."

Sure, the current testing seems to be dependent on the ability to inject this factory though unless I'm overlooking something?

This comment has been minimized.

@juanvallejo

juanvallejo Mar 7, 2018

Member

Usually we take the bits we need from the factory (any methods) and tack them on to the options struct in Complete(): o.ClientSet = f.KubernetesClientSet (builder logic also usually takes place in Complete, with the resulting infos being aggregated)

This comment has been minimized.

@PhilipGough

PhilipGough Mar 7, 2018

Author Contributor

Great, ok I can take a look at doing that in separate pr or commit

This comment has been minimized.

@juanvallejo

juanvallejo Mar 7, 2018

Member

A separate pr sounds great for this. Thanks

},
},
}
if contains(key, o.Keys) {

This comment has been minimized.

@juanvallejo

juanvallejo Mar 6, 2018

Member

move this check to the top of this loop

},
},
}
if contains(key, o.Keys) {

This comment has been minimized.

@juanvallejo

juanvallejo Mar 6, 2018

Member

same as above

return err
for _, s := range secrets {
for key := range s.Data {
envVar := v1.EnvVar{

This comment has been minimized.

@juanvallejo

juanvallejo Mar 6, 2018

Member

This logic looks the same in the configmaps loop, this inner loop could be placed in a separate function that returns []v1.EnvVar

This comment has been minimized.

@PhilipGough

PhilipGough Mar 7, 2018

Author Contributor

Sorry, typo by me, the struct contents should be different for both resources

@PhilipGough PhilipGough force-pushed the PhilipGough:keys-from-cm-patch branch 2 times, most recently from 46e9cd9 to f9c9f86 Mar 7, 2018

@juanvallejo

This comment has been minimized.

Copy link
Member

juanvallejo commented Mar 7, 2018

/lgtm

cc @soltysh for approval

@juanvallejo

This comment has been minimized.

Copy link
Member

juanvallejo commented Mar 21, 2018

/test pull-kubernetes-integration

Latest()
}

infos, err := b.Do().Infos()

This comment has been minimized.

@soltysh

soltysh Mar 30, 2018

Contributor

This operation does not belong in Complete. Complete should fill in the necessary bits of the EnvOptions struct so that they are then validated and an actual operation is performed in Run. This should be moved to Run.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Apr 11, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@PhilipGough @juanvallejo @soltysh

Pull Request Labels
  • sig/cli: Pull Request will be escalated to these SIGs if needed.
  • priority/important-longterm: Escalate to the pull request owners; move out of the milestone after 1 attempt.
  • kind/feature: New functionality.
Help
@dims

This comment has been minimized.

Copy link
Member

dims commented Apr 30, 2018

/uncc @dims

@PhilipGough PhilipGough force-pushed the PhilipGough:keys-from-cm-patch branch from 0d09e82 to a9401c8 May 20, 2018

@PhilipGough PhilipGough force-pushed the PhilipGough:keys-from-cm-patch branch from a9401c8 to 120e878 May 20, 2018

@soltysh
Copy link
Contributor

soltysh left a comment

/lgtm
/approve

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 21, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 21, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@PhilipGough PhilipGough force-pushed the PhilipGough:keys-from-cm-patch branch from 120e878 to 027d15e May 22, 2018

@k8s-ci-robot k8s-ci-robot added lgtm and removed lgtm labels May 22, 2018

@soltysh
Copy link
Contributor

soltysh left a comment

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 22, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, juanvallejo, 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:

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

3 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 23, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@PhilipGough

This comment has been minimized.

Copy link
Contributor Author

PhilipGough commented May 23, 2018

/test all

Tests seem to be stuck in pending even though they passed

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 23, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 23, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 23, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 23, 2018

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

@k8s-github-robot k8s-github-robot merged commit 420071d into kubernetes:master May 23, 2018

15 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation PhilipGough authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.