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

Liujingfang1 · 2019-02-15T19:22:40Z

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 · 2019-02-15T19:22:48Z

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 · 2019-02-15T19:24:00Z

/sig-cli
/assign @pwittrock @seans3 @liggitt @monopole

pkg/kubectl/cmd/apply/apply.go

liggitt · 2019-02-16T02:23:54Z

staging/src/k8s.io/cli-runtime/pkg/genericclioptions/resource/builder.go

 	Recursive bool
 }

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


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?

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.

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

Liujingfang1 · 2019-02-19T21:44:17Z

/assign @soltysh @monopole

Liujingfang1 · 2019-02-19T21:44:45Z

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?

pwittrock · 2019-02-20T00:32:29Z

pkg/kubectl/cmd/delete/delete_flags.go


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


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

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?

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

How about

// Not using helpers.go it since provides function to add '-k' for FileNameOptions, but not FileNameFlags

pwittrock · 2019-02-20T00:35:45Z

pkg/kubectl/cmd/create/create.go

@@ -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) {


Is it possible to keep printing the helper message?

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 }

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

pwittrock · 2019-02-20T00:39:23Z

test/cmd/apply.sh

@@ -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


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

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

pwittrock · 2019-02-20T00:41:39Z

pkg/kubectl/cmd/apply/apply.go

@@ -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.


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

pwittrock · 2019-02-21T17:39:25Z

/ok-to-test

pwittrock · 2019-02-26T18:19:37Z

/retest

staging/src/k8s.io/cli-runtime/pkg/genericclioptions/resource/builder_test.go

pwittrock · 2019-02-26T20:02:08Z

staging/src/k8s.io/cli-runtime/pkg/genericclioptions/resource/builder_test.go

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


Add an error for specifying the kustomization file directly

Added one test with

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

Liujingfang1 · 2019-02-26T23:29:30Z

/test pull-kubernetes-e2e-gce

pwittrock · 2019-02-27T00:37:00Z

/approve
/lgtm

pwittrock · 2019-02-27T00:37:37Z

/lgtm
/approve

pwittrock · 2019-02-27T00:37:57Z

/hold

Added hold for @apelisse

pwittrock · 2019-02-27T00:38:50Z

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

apelisse · 2019-02-27T00:40:17Z

/approve

pwittrock · 2019-02-27T00:42:44Z

/assign @dims

For staging/publishing

dims · 2019-02-27T01:10:04Z

/approve

k8s-ci-robot · 2019-02-27T01:10:29Z

[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:

~~hack/testdata/OWNERS~~ [apelisse,dims,pwittrock]
~~pkg/kubectl/OWNERS~~ [apelisse]
~~staging/publishing/OWNERS~~ [dims]
~~staging/src/k8s.io/cli-runtime/OWNERS~~ [pwittrock]
~~staging/src/k8s.io/sample-cli-plugin/Godeps/OWNERS~~ [apelisse]
~~test/cmd/OWNERS~~ [apelisse,pwittrock]

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

pwittrock · 2019-02-27T01:16:07Z

/hold cancel

k8s-ci-robot requested review from dshulyak and eparis February 15, 2019 19:23

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 15, 2019

k8s-ci-robot assigned liggitt, pwittrock and seans3 Feb 15, 2019

liggitt reviewed Feb 16, 2019

View reviewed changes

Liujingfang1 force-pushed the kflag branch from 9b206bc to 7a37879 Compare February 19, 2019 19:13

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 19, 2019

k8s-ci-robot assigned monopole and soltysh Feb 19, 2019

pwittrock reviewed Feb 20, 2019

View reviewed changes

apelisse reviewed Feb 26, 2019

View reviewed changes

staging/src/k8s.io/cli-runtime/pkg/genericclioptions/resource/builder_test.go Outdated Show resolved Hide resolved

fix the failed test

f55d87a

Liujingfang1 force-pushed the kflag branch from 731e637 to f55d87a Compare February 26, 2019 18:36

pwittrock reviewed Feb 26, 2019

View reviewed changes

address comments

127c2f1

Liujingfang1 force-pushed the kflag branch from 92dfa16 to 127c2f1 Compare February 26, 2019 22:13

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2019

k8s-ci-robot unassigned soltysh, monopole and seans3 Feb 27, 2019

k8s-ci-robot assigned dims Feb 27, 2019

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2019

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

hoegaarden mentioned this pull request Mar 12, 2019

Minor release notes formatting issue for code blocks kubernetes/release#681

Closed

spiffxp mentioned this pull request Mar 22, 2019

format error in CHANGELOG-1.14.md #75543

Merged

liggitt added this to the v1.14 milestone Jun 10, 2019

liggitt removed the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Liujingfang1 commented Feb 15, 2019 •

edited by spiffxp

k8s-ci-robot commented Feb 15, 2019

Liujingfang1 commented Feb 15, 2019 •

edited

liggitt Feb 16, 2019

Liujingfang1 Feb 19, 2019

Liujingfang1 Feb 19, 2019

Liujingfang1 commented Feb 19, 2019

Liujingfang1 commented Feb 19, 2019

pwittrock Feb 20, 2019

Liujingfang1 Feb 20, 2019

pwittrock Feb 21, 2019

Liujingfang1 Feb 21, 2019

pwittrock Feb 20, 2019

Liujingfang1 Feb 20, 2019

pwittrock Feb 21, 2019

pwittrock Feb 20, 2019

Liujingfang1 Feb 20, 2019

pwittrock Feb 21, 2019

pwittrock Feb 20, 2019

pwittrock commented Feb 21, 2019

pwittrock commented Feb 26, 2019

pwittrock Feb 26, 2019

Liujingfang1 Feb 26, 2019

Liujingfang1 commented Feb 26, 2019

pwittrock commented Feb 27, 2019

pwittrock commented Feb 27, 2019

pwittrock commented Feb 27, 2019 •

edited

pwittrock commented Feb 27, 2019

apelisse commented Feb 27, 2019

pwittrock commented Feb 27, 2019 •

edited

dims commented Feb 27, 2019

k8s-ci-robot commented Feb 27, 2019

pwittrock commented Feb 27, 2019

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

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

Conversation

Liujingfang1 commented Feb 15, 2019 • edited by spiffxp

k8s-ci-robot commented Feb 15, 2019

Liujingfang1 commented Feb 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Liujingfang1 commented Feb 19, 2019

Liujingfang1 commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Feb 21, 2019

pwittrock commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Liujingfang1 commented Feb 26, 2019

pwittrock commented Feb 27, 2019

pwittrock commented Feb 27, 2019

pwittrock commented Feb 27, 2019 • edited

pwittrock commented Feb 27, 2019

apelisse commented Feb 27, 2019

pwittrock commented Feb 27, 2019 • edited

dims commented Feb 27, 2019

k8s-ci-robot commented Feb 27, 2019

pwittrock commented Feb 27, 2019

Liujingfang1 commented Feb 15, 2019 •

edited by spiffxp

Liujingfang1 commented Feb 15, 2019 •

edited

pwittrock commented Feb 27, 2019 •

edited

pwittrock commented Feb 27, 2019 •

edited