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

add -k flag in cli-runtime and kubectl to process kustomization directories #74140

Merged
merged 7 commits into from Feb 27, 2019

Conversation

@Liujingfang1
Copy link
Contributor

Liujingfang1 commented Feb 15, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR implements the 2nd Customize integration KEP

Which issue(s) this PR fixes:
Fixes kubernetes/enhancements#633

Special notes for your reviewer:
This PR adds -k flag into FilenameFlags in cli-runtime.
Kubectl consumes FilenameFlags in different sub commands.

Does this PR introduce a user-facing change?:
yes

The examples in kubectl apply/get/delete are updated to support `-k` which uses a `kustomization.yaml` file
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 15, 2019

Hi @Liujingfang1. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Liujingfang1

This comment has been minimized.

Copy link
Contributor Author

Liujingfang1 commented Feb 15, 2019

Show resolved Hide resolved pkg/kubectl/cmd/apply/apply.go Outdated
Recursive bool
}

func (o *FilenameOptions) Validate() error {
if len(o.Filenames) > 0 && len(o.Kustomize) > 0 {

This comment has been minimized.

@liggitt

liggitt Feb 16, 2019

Member

it's likely many FilenameOptions/Builder users will not call this new method. should this validation go into Builder#FilenameParam and append to Builder#errs instead?

This comment has been minimized.

@Liujingfang1

Liujingfang1 Feb 19, 2019

Author Contributor

I added two functions to FilenameOptions: Validate() and IsSourceEmpty().

Validate() is to make sure there is no -f and -k conflicts. This function is needed wherever FilenameOptions is used. So it makes sense to move it into Builder#FilenameParam.

IsSourceEmpty() is to check both -f and -k are not set. This is used to make sure at least one of -f and -k is specified. There are subcommands such as apply. Previously, -f is required in those subcommands. So we need to change them to require one of -f and -k.

This comment has been minimized.

@Liujingfang1

Liujingfang1 Feb 19, 2019

Author Contributor

validate now is triggered inside FilenameParam function. In addition, I renamed IsSourceEmpty() to RequireFilenameOrKustomize.

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:kflag branch from 9b206bc to 7a37879 Feb 19, 2019

@Liujingfang1

This comment has been minimized.

Copy link
Contributor Author

Liujingfang1 commented Feb 19, 2019

@Liujingfang1

This comment has been minimized.

Copy link
Contributor Author

Liujingfang1 commented Feb 19, 2019

By this change, -k is added to all kubectl subcommands that are with a -f flag. Does it make sense? Do we need to restrict -k to a smaller set of subcommands?


return &DeleteFlags{
FileNameFlags: &genericclioptions.FileNameFlags{Usage: usage, Filenames: &filenames, Recursive: &recursive},
FileNameFlags: &genericclioptions.FileNameFlags{Usage: usage, Filenames: &filenames, Kustomize: &kustomize, Recursive: &recursive},

This comment has been minimized.

@pwittrock

pwittrock Feb 20, 2019

Member

Why doesn't this use the helpers.go? We should at least document it.

This comment has been minimized.

@Liujingfang1

Liujingfang1 Feb 20, 2019

Author Contributor

DeleteFlags composes common flag struct used for commands requiring deletion logic, such as kubectl delete and kubectl apply. It contains FileNameFlags, which is then converted to FileNameOptions.

While in other subcommand options, FileNameOptions is directly used instead of FileNameFlags. helpers.go contains functions to add -k and -f for FileNameOptions, but not for FileNameFlags.

In the future, I think the logic of adding flags should be moved to cli-runtime. Do we convert all current FileNameOptions to FileNameFlags or keep both of them? I'm not sure. @juanvallejo What do you think?

This comment has been minimized.

@pwittrock

pwittrock Feb 21, 2019

Member

Please add a comment to this code explaining why it doesn't use helpers.go

This comment has been minimized.

@Liujingfang1

Liujingfang1 Feb 21, 2019

Author Contributor

How about

// Not using helpers.go it since provides function to add '-k' for FileNameOptions, but not FileNameFlags
@@ -103,11 +103,6 @@ func NewCmdCreate(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cob
Long: createLong,
Example: createExample,
Run: func(cmd *cobra.Command, args []string) {
if cmdutil.IsFilenameSliceEmpty(o.FilenameOptions.Filenames) {

This comment has been minimized.

@pwittrock

pwittrock Feb 20, 2019

Member

Is it possible to keep printing the helper message?

This comment has been minimized.

@Liujingfang1

Liujingfang1 Feb 20, 2019

Author Contributor

We can keep it. The change would be like

if cmdutil.IsFilenameSliceEmpty(o.FilenameOptions.Filenames, o.FilenameOptions.Kustomize) {
	ioStreams.ErrOut.Write([]byte("Error: must specify one of -f and -k\n\n"))
	defaultRunFunc := cmdutil.DefaultSubCommandRun(ioStreams.ErrOut)
	defaultRunFunc(cmd, args)
	return
}

This comment has been minimized.

@pwittrock

pwittrock Feb 21, 2019

Member

SGTM. If we want to change this behavior in the future it should be a separate discussion.

@@ -223,6 +223,21 @@ __EOF__
# cleanup
kubectl delete -f hack/testdata/service-revision2.yaml "${kube_flags[@]}"

## kubectl apply -k somedir
kubectl apply -k hack/testdata/kustomize

This comment has been minimized.

@pwittrock

pwittrock Feb 20, 2019

Member

lets add a test for every command that supports this if possible

This comment has been minimized.

@Liujingfang1

Liujingfang1 Feb 20, 2019

Author Contributor

How about adding tests to the existing test files for apply, diff, create, delete, get?

This comment has been minimized.

@pwittrock
@@ -115,6 +115,9 @@ var (
# Apply the configuration in pod.json to a pod.
kubectl apply -f ./pod.json
# Apply resources from a directory containing kustomization.yaml.

This comment has been minimized.

@pwittrock

pwittrock Feb 20, 2019

Member
# Apply resources from a directory containing kustomization.yaml. - e.g. dir/kustomization.yaml
kubectl apply -k dir/
@pwittrock

This comment has been minimized.

Copy link
Member

pwittrock commented Feb 21, 2019

/ok-to-test

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:kflag branch from dc2fead to beacc87 Feb 26, 2019

@pwittrock

This comment has been minimized.

Copy link
Member

pwittrock commented Feb 26, 2019

/retest

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:kflag branch from 731e637 to f55d87a Feb 26, 2019

directory: "../../../artifacts/kustomization/should-not-load.yaml",
expectErr: true,
errMsg: "must be a directory to be a root",
},

This comment has been minimized.

@pwittrock

pwittrock Feb 26, 2019

Member

Add an error for specifying the kustomization file directly

This comment has been minimized.

@Liujingfang1

Liujingfang1 Feb 26, 2019

Author Contributor

Added one test with

directory: filepath.Join(dir, "kustomization.yaml"),

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:kflag branch from 92dfa16 to 127c2f1 Feb 26, 2019

@Liujingfang1

This comment has been minimized.

Copy link
Contributor Author

Liujingfang1 commented Feb 26, 2019

/test pull-kubernetes-e2e-gce

@pwittrock

This comment has been minimized.

Copy link
Member

pwittrock commented Feb 27, 2019

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 27, 2019

@pwittrock

This comment has been minimized.

Copy link
Member

pwittrock commented Feb 27, 2019

/lgtm
/approve

@pwittrock

This comment has been minimized.

Copy link
Member

pwittrock commented Feb 27, 2019

/hold

Added hold for @apelisse

@pwittrock

This comment has been minimized.

Copy link
Member

pwittrock commented Feb 27, 2019

/unassign @soltysh
/unassign @monopole
/unassign @seans3

@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Feb 27, 2019

/approve

@pwittrock

This comment has been minimized.

Copy link
Member

pwittrock commented Feb 27, 2019

/assign @dims

For staging/publishing

@dims

This comment has been minimized.

Copy link
Member

dims commented Feb 27, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 27, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, dims, Liujingfang1, pwittrock

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

@pwittrock

This comment has been minimized.

Copy link
Member

pwittrock commented Feb 27, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 81e6407 into kubernetes:master Feb 27, 2019

16 checks passed

cla/linuxfoundation Liujingfang1 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-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Job succeeded.
Details
tide In merge pool.
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.