Add apply set-last-applied subcommand #41694

shiywang · 2017-02-18T16:26:16Z

implement part of kubernetes/community#287, will rebase after #41699 got merged, EDIT: since bug output format has been confirmed, will update the behavior of output format soon
cc @kubernetes/sig-cli-pr-reviews @adohe @pwittrock

Support kubectl apply set-last-applied command to update the applied-applied-configuration annotation

k8s-ci-robot · 2017-02-18T16:26:17Z

Hi @shiywang. 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 @k8s-bot 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.

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. I understand the commands that are listed here.

k8s-reviewable · 2017-02-18T16:27:21Z

This change is

shiywang · 2017-02-18T16:33:57Z

why cc @kubernetes/sig-cli-pr-reviews doesn't assign to right people ?

pwittrock · 2017-02-18T17:32:50Z

cc'ing doesn't set the reviewers, it just makes sure the right people get notified.

adohe-zz · 2017-02-20T13:35:30Z

pkg/kubectl/cmd/apply_set_last_applied.go

+	"k8s.io/kubernetes/pkg/kubectl/resource"
+)
+
+type SetLastAppliedOptions struct {


this code snippet duplicate with view-last-applied, can refactor if that get merged.

ok, I'll refactor it when all sub-commands get merged.

adohe-zz · 2017-02-20T13:42:27Z

pkg/kubectl/cmd/apply_set_last_applied.go

+		Long:    applySetLastAppliedLong,
+		Example: applySetLastAppliedExample,
+		Run: func(cmd *cobra.Command, args []string) {
+			cmdutil.CheckErr(options.RunApplySetLastApplied(f, args, cmd, out, err, options))


I would like this also follow Complete Validate and Run convention.

forget that convention again, yuck.... will update soon

adohe-zz · 2017-02-20T13:46:20Z

pkg/kubectl/cmd/apply_set_last_applied.go

+	return cmd
+}
+
+func (o *SetLastAppliedOptions) RunApplySetLastApplied(f cmdutil.Factory, args []string, cmd *cobra.Command, out, errOut io.Writer, options *SetLastAppliedOptions) error {


pls avoid direct cobra dependency.

adohe-zz · 2017-02-20T15:48:30Z

pkg/kubectl/cmd/apply_set_last_applied.go

+
+	applySetLastAppliedExample = templates.Examples(`
+		# Set the current file's content to the latest-applied-configuration of this resource.
+		kubectl apply set-last-applied -f deploy.yaml`)


which resource to be updated? identified by deploy.yaml?

adohe-zz · 2017-02-20T15:52:17Z

pkg/kubectl/cmd/apply_set_last_applied.go

+		annotationsMap[annotations.LastAppliedConfigAnnotation] = string(localFile)
+		metadataMap["annotations"] = annotationsMap
+		objMap["metadata"] = metadataMap
+		jsonString, err := json.Marshal(objMap)


I am afraid this is not correct here, we should calculate the final patch between the current object and object with annotation updated.

so, it's like run apply -f first, get the result object then calculate the final patch between current object and the result object I got before ? currently I think I just set the file's object to annotations then do a json-merge-patch

@adohe What is the result of the incorrect behavior? AFAICT this send just a regular mergepatch with the single annotation instead of calculating the full diff between objects. Is this incorrect or just not the recommended style?

generally this does the same thing as kubectl annotate, i would think the way annotate use is more clean.

@adohe Hm. I am not sure I totally agree. WDYT of of leaving it as is for now, so this makes feature freeze (tomorrow), and we can follow up if we decide we want to change it.

adohe-zz · 2017-02-20T15:52:44Z

pkg/kubectl/cmd/apply_set_last_applied.go

+			}
+		}
+
+		if !dryRun {


should add dry-run handling.

Should dryrun just print out the patch string that would be sent? Does dryrun make sense in this context at all? WDYT @adohe ?

dry-run means just print the object that would be sent~yep that would make sense print out the patch when dry-run

pwittrock · 2017-02-21T16:42:13Z

pkg/kubectl/cmd/apply_set_last_applied.go

+		if err != nil {
+			return err
+		}
+		annotationsMap[annotations.LastAppliedConfigAnnotation] = string(localFile)


If there are no changes, we shouldn't do a patch

pwittrock · 2017-02-21T16:43:13Z

pkg/kubectl/cmd/apply_set_last_applied.go

+			}
+
+			if cmdutil.ShouldRecord(cmd, info) {
+				if patch, patchType, err := cmdutil.ChangeResourcePatch(info, f.Command()); err == nil {


Why is this a separate patch command instead of part of the first patch command?

I am not really sure we should update this at all since it isn't updating the object itself. Lets not do this for now.

pwittrock · 2017-02-21T16:43:19Z

pkg/kubectl/cmd/apply_set_last_applied.go

+				return err
+			}
+
+			if cmdutil.ShouldRecord(cmd, info) {


Add a comment

pwittrock · 2017-02-21T16:44:10Z

pkg/kubectl/cmd/apply_set_last_applied.go

+
+var (
+	applySetLastAppliedLong = templates.LongDesc(`
+		Set the latest last-applied-configuration annotations by file.`)


Set the latest last-applied-configuration annotations by setting it to match the contents of a file. This results in the last-applied-configuration being updated as though 'kubectl apply -f <file>' was run, without updating any other parts of the object.

pwittrock · 2017-02-21T16:53:15Z

pkg/kubectl/cmd/apply_set_last_applied.go

+		Set the latest last-applied-configuration annotations by file.`)
+
+	applySetLastAppliedExample = templates.Examples(`
+		# Set the current file's content to the latest-applied-configuration of this resource.


Set the last-applied-configuration of a resource to match the contents of a file.

pwittrock · 2017-02-21T16:57:42Z

pkg/kubectl/cmd/apply_set_last_applied.go

+		annotationsMap[annotations.LastAppliedConfigAnnotation] = string(localFile)
+		metadataMap["annotations"] = annotationsMap
+		objMap["metadata"] = metadataMap
+		jsonString, err := json.Marshal(objMap)


@adohe What is the result of the incorrect behavior? AFAICT this send just a regular mergepatch with the single annotation instead of calculating the full diff between objects. Is this incorrect or just not the recommended style?

pwittrock · 2017-02-21T17:03:01Z

pkg/kubectl/cmd/apply_set_last_applied.go

+			return err
+		}
+
+		objMap := map[string]map[string]map[string]string{}


plz pull the patch calculation into its own function. e.g.

func (o *SetLastAppliedOptions) getPatch(info) string, error { ... return json.Marshal(objMap) }

pwittrock · 2017-02-21T17:04:41Z

pkg/kubectl/cmd/apply_set_last_applied.go

+			return err
+		}
+
+		if err := info.Get(); err != nil {


pull this into the validation function. Add a comment explaining why we do this.

// Verify the object exists in the cluster before trying to patch it.

pwittrock · 2017-02-21T17:06:21Z

pkg/kubectl/cmd/apply_set_last_applied.go

+			}
+		}
+
+		if !dryRun {


Should dryrun just print out the patch string that would be sent? Does dryrun make sense in this context at all? WDYT @adohe ?

pwittrock · 2017-02-22T00:20:25Z

pkg/kubectl/cmd/apply_test.go

-	filenameRC    = "../../../test/fixtures/pkg/kubectl/cmd/apply/rc.yaml"
-	filenameSVC   = "../../../test/fixtures/pkg/kubectl/cmd/apply/service.yaml"
-	filenameRCSVC = "../../../test/fixtures/pkg/kubectl/cmd/apply/rc-service.yaml"
+	filenameRC        = "../../../test/fixtures/pkg/kubectl/cmd/apply/rc.yaml"


also add an e2e test that does

kubectl apply -f deployment1.yaml // contains replicas = 2 kubectl apply view-last-applied -f deploymen1.yaml // check the last-applied matches expectations kubectl apply set-last-applied -f deployment2.yaml // doesn't have replicas kubectl apply view-last-applied -f deploymen1.yaml // check last-applied has been updated kubectl scale deployment ... // set replicas = 3 kubectl apply -f deployment3.yaml // doesn't have replicas, and changes the images kubectl get -f deployment3.yaml // verify replicas still = 3 and image has been updated

adohe-zz · 2017-02-22T13:36:46Z

@pwittrock I am wondering whether we should allow set last-applied on objects which are not managed by apply. wdyt?

pwittrock · 2017-02-23T16:05:55Z

@shiywang Please rebase to resolve conflicts.

shiywang · 2017-02-23T16:08:07Z

@pwittrock ok, great, will do that

pwittrock · 2017-02-23T15:55:55Z

pkg/kubectl/cmd/apply_set_last_applied.go

+	options := &SetLastAppliedOptions{Out: out, ErrOut: err}
+	cmd := &cobra.Command{
+		Use:     "set-last-applied -f FILENAME",
+		Short:   "Set latest last-applied-configuration annotations of by file",


Awkward language. Use:

Set the last-applied-configuration annotation on a live object to match the contents of a file.

pwittrock · 2017-02-23T15:58:20Z

pkg/kubectl/cmd/apply_set_last_applied.go

+		# Set the last-applied-configuration of a resource to match the contents of a file.
+		kubectl apply set-last-applied -f deploy.yaml
+
+		# Set the last-applied-configuration of resourcesto match the contents of files in a directory.


typo + awkward:

Execute set-last-applied against each configuration file in a directory.

pwittrock · 2017-02-23T15:59:49Z

pkg/kubectl/cmd/apply_set_last_applied.go

+			return cmdutil.AddSourceToErr(fmt.Sprintf("retrieving current configuration of:\n%v\nfrom server for:", info), info.Source, err)
+		}
+		if oringalBuf == nil {
+			return cmdutil.AddSourceToErr(fmt.Sprintf("no last-applied-configuration annotation found on resource: %s\n", info.Name), info.Source, err)


Lets add a flag --create-annotation that will allow this to be set if it is missing. Default should be false.

pwittrock · 2017-02-23T16:00:34Z

pkg/kubectl/cmd/apply_set_last_applied.go

+			o.PatchBufferList = append(o.PatchBufferList, patchBuf)
+			o.InfoList = append(o.InfoList, info)
+		} else {
+			fmt.Fprintf(o.Out, "set-last-applied %s: no valid changes saved, won't be patch\n", info.Name)


Awkward:

set-last-applied %s: no changes required.\n

pwittrock · 2017-02-23T16:03:08Z

pkg/kubectl/cmd/apply_test.go

+			expectedOut: "replicationcontroller/test-rc\n",
+		},
+		{
+			name:        "set with no-exist object",


Add test for the annotation does not exist on the live object.

pwittrock · 2017-02-23T16:03:29Z

pkg/kubectl/cmd/apply_test.go

+		{
+			name:        "set with no-exist object",
+			filePath:    filenameNoExistRC,
+			expectedErr: "Error from server (NotFound): the server could not find the requested resource (get replicationcontrollers no-exist)",


Add test for a json file

pwittrock · 2017-02-23T16:03:39Z

pkg/kubectl/cmd/apply_test.go

+		{
+			name:        "set with no-exist object",
+			filePath:    filenameNoExistRC,
+			expectedErr: "Error from server (NotFound): the server could not find the requested resource (get replicationcontrollers no-exist)",


Add test for a directory of files

pwittrock · 2017-02-23T16:50:23Z

pkg/kubectl/cmd/apply_set_last_applied.go

+		# Set the last-applied-configuration of resourcesto match the contents of files in a directory.
+		kubectl apply set-last-applied -f path/
+
+		# Set the last-applied-configuration of a resource to match the contents of a file, will create if resource doesn't have one.


will create the annotation if it does not already exist

pwittrock · 2017-02-23T16:51:43Z

pkg/kubectl/cmd/apply_set_last_applied.go

+			return cmdutil.AddSourceToErr(fmt.Sprintf("retrieving current configuration of:\n%v\nfrom server for:", info), info.Source, err)
+		}
+		if oringalBuf == nil && !o.CreateAnnotation {
+			return cmdutil.UsageError(cmd, "no last-applied-configuration annotation found on resource: %s, please use '--create-annotation' to create one", info.Name)


please use '--create-annotation' to create one

How about

to create the annotation, run the command with --create-annotation

shiywang · 2017-02-23T23:35:38Z

@pwittrock ptal ?

pwittrock · 2017-02-24T01:12:42Z

@k8s-bot test this

k8s-ci-robot · 2017-02-24T01:56:38Z

@shiywang: you can't request testing unless you are a kubernetes member.

In response to this comment:

@k8s-bot test this

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. I understand the commands that are listed here.

shiywang · 2017-02-24T02:02:03Z

@pwittrock hi, just can not reach you on slack, sorry, just forgot update bazel last night, could you please let bot tests again ?

mengqiy · 2017-02-24T02:05:17Z

@k8s-bot ok to test

update update code update unit tests hack/update remove spew update bazel updated add comments remove unused parameter remove hardcode bump unit tests add new flags add unit tests add bazel genreate doc

shiywang · 2017-02-24T03:12:58Z

@pwittrock docs genreated, ptal

shiywang · 2017-02-24T04:31:03Z

@k8s-bot unit test this

pwittrock · 2017-02-24T16:32:48Z

/lgtm

pwittrock · 2017-02-24T16:32:52Z

/approve

k8s-github-robot · 2017-02-24T16:33:48Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: pwittrock, shiywang

Needs approval from an approver in each of these OWNERS Files:

~~docs/OWNERS~~ [pwittrock]
~~hack/verify-flags/OWNERS~~ [pwittrock]
~~pkg/kubectl/OWNERS~~ [pwittrock]
~~test/fixtures/pkg/kubectl/OWNERS~~ [pwittrock]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-02-27T07:16:51Z

Automatic merge from submit-queue (batch tested with PRs 42044, 41694, 41927, 42050, 41987)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 18, 2017

k8s-github-robot assigned dshulyak Feb 18, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Feb 18, 2017

shiywang force-pushed the apply-set1 branch from a36c7d5 to d471d0e Compare February 18, 2017 16:28

shiywang force-pushed the apply-set1 branch 2 times, most recently from 0a2c435 to 1b40c42 Compare February 18, 2017 16:31

shiywang force-pushed the apply-set1 branch from 1b40c42 to 950b684 Compare February 18, 2017 16:33

pwittrock assigned pwittrock and unassigned dshulyak Feb 18, 2017

adohe-zz reviewed Feb 20, 2017

View reviewed changes

pwittrock reviewed Feb 22, 2017

View reviewed changes

shiywang force-pushed the apply-set1 branch 2 times, most recently from 760a00b to b0c0ff2 Compare February 22, 2017 15:57

shiywang force-pushed the apply-set1 branch from 78c8fa6 to b292e7c Compare February 23, 2017 16:46

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017

pwittrock reviewed Feb 23, 2017

View reviewed changes

shiywang force-pushed the apply-set1 branch from b292e7c to 23f8a9e Compare February 23, 2017 17:45

pwittrock removed the release-note-label-needed label Feb 24, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 24, 2017

shiywang force-pushed the apply-set1 branch from 23f8a9e to f516a56 Compare February 24, 2017 01:56

add apply-set-last-applied subcommand

9bace3e

update update code update unit tests hack/update remove spew update bazel updated add comments remove unused parameter remove hardcode bump unit tests add new flags add unit tests add bazel genreate doc

shiywang force-pushed the apply-set1 branch from f516a56 to 9bace3e Compare February 24, 2017 03:12

k8s-github-robot added the kind/old-docs label Feb 24, 2017

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2017

k8s-github-robot merged commit e6ecfd2 into kubernetes:master Feb 27, 2017

shiywang mentioned this pull request Mar 31, 2017

Make apply to it's own package #43886

Closed

4 tasks

shiywang deleted the apply-set1 branch April 4, 2017 07:21

shiywang mentioned this pull request Apr 4, 2017

Add kubectl apply edit-last-applied subcommand #42256

Merged

Add apply set-last-applied subcommand #41694

Add apply set-last-applied subcommand #41694

Conversation

shiywang commented Feb 18, 2017 • edited by pwittrock

k8s-ci-robot commented Feb 18, 2017

k8s-reviewable commented Feb 18, 2017

shiywang commented Feb 18, 2017

pwittrock commented Feb 18, 2017

Choose a reason for hiding this comment

shiywang Feb 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang Feb 20, 2017 • edited

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

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

adohe-zz commented Feb 22, 2017

pwittrock commented Feb 23, 2017

shiywang commented Feb 23, 2017

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

shiywang commented Feb 23, 2017 • edited

pwittrock commented Feb 24, 2017

k8s-ci-robot commented Feb 24, 2017

shiywang commented Feb 24, 2017 • edited

mengqiy commented Feb 24, 2017

shiywang commented Feb 24, 2017

shiywang commented Feb 24, 2017

pwittrock commented Feb 24, 2017

pwittrock commented Feb 24, 2017

k8s-github-robot commented Feb 24, 2017

k8s-github-robot commented Feb 27, 2017

shiywang commented Feb 18, 2017 •

edited by pwittrock

shiywang Feb 20, 2017 •

edited

shiywang Feb 20, 2017 •

edited

shiywang commented Feb 23, 2017 •

edited

shiywang commented Feb 24, 2017 •

edited