Skip to content

Add kubectl apply edit-last-applied subcommand #42256

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 26, 2017
Merged

Add kubectl apply edit-last-applied subcommand #42256

merged 1 commit into from
May 26, 2017

Conversation

@shiywang
Copy link
Contributor

@shiywang shiywang commented Feb 28, 2017 •
edited
Loading

third command of kubernetes/community#287
Fixes #44905
@pwittrock @adohe @ymqytw @kubernetes/sig-cli-feature-requests could you guys have an early review ? cause some of feature I'm not sure about, will add unit tests if you think it's ok.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 28, 2017

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-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 28, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Feb 28, 2017
@deads2k
Copy link
Contributor

deads2k commented Feb 28, 2017

@kubernetes/sig-cli-feature-requests

@shiywang shiywang changed the title [WIP] add kubectl apply edit-last-applied subcommand Add kubectl apply edit-last-applied subcommand Mar 1, 2017
@k8s-reviewable
Copy link

k8s-reviewable commented Mar 2, 2017

This change is Reviewable

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 6, 2017
@shiywang
Copy link
Contributor Author

shiywang commented Mar 7, 2017 •
edited
Loading

@pwittrock @adohe also add some unit tests, ptal

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2017
@dims
Copy link
Member

dims commented Mar 8, 2017

@k8s-bot ok to test

@dims
Copy link
Member

dims commented Mar 10, 2017

@shiywang : can you please see the broken tests?

@shiywang shiywang force-pushed the edit branch 2 times, most recently from 6a922a4 to fbebba8 Compare March 11, 2017 07:06
@shiywang
Copy link
Contributor Author

shiywang commented Mar 11, 2017

@dims sure, done

@dims
Copy link
Member

dims commented Mar 29, 2017

@pwittrock @adohe @ymqytw @kubernetes/sig-cli-feature-requests Can one of you please chime in?

@mengqiy
Copy link
Member

mengqiy commented Mar 29, 2017

/assign @ymqytw

@dims
Copy link
Member

dims commented Mar 29, 2017

/unassign @dims

@shiywang shiywang mentioned this pull request Mar 31, 2017
Closed
4 tasks
@mengqiy
Copy link
Member

mengqiy commented Mar 31, 2017

According to kubectl convention

Validate: performs validation on the struct fields and returns appropriate errors.

Validate it should not have non-validation stuff in it. Leave it empty if there is nothing to do with validation.

I will revisit this tomorrow.

@mengqiy
Copy link
Member

mengqiy commented Mar 31, 2017

/release-note

@shiywang
Copy link
Contributor Author

shiywang commented May 24, 2017 •
edited
Loading

@alexandercampbell thanks for you comments

You could fall back to VISUAL as well, although I'm not sure anybody sets that variable any more.

I think current is fine, this command is almost same as kubectl edit in edit way

How is this verified?

if not, return Error from server (NotFound) I guess

Where is this change used?

used in function RunSetLastApplied

Sorry, how can we be sure that tempInfos has one item?

there's only one last-applied-configuration object per objects

@shiywang
Copy link
Contributor Author

shiywang commented May 24, 2017

@mengqiy @liggitt updated, ptal

liggitt
liggitt reviewed May 24, 2017
View reviewed changes
pkg/kubectl/cmd/apply_set_last_applied.go Outdated
Copy link
Member

@liggitt liggitt May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer a list of structs containing the patch content and patch type over parallel lists:

PatchBufferList    []PatchBuffer
...

type PatchBuffer struct {
	Patch     []byte
	PatchType types.PatchType
}

@shiywang
Copy link
Contributor Author

shiywang commented May 24, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@shiywang
Copy link
Contributor Author

shiywang commented May 24, 2017

@liggitt updated, not sure why pull-kubernetes-federation-e2e-gce always failed, ptal
@mengqiy @fabianofranz @pwittrock ping for approval

@liggitt
Copy link
Member

liggitt commented May 24, 2017

LGTM

@mengqiy
Copy link
Member

mengqiy commented May 24, 2017

Thanks for your review.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2017
@shiywang
Copy link
Contributor Author

shiywang commented May 25, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

seh
seh reviewed May 25, 2017
View reviewed changes
pkg/kubectl/cmd/apply_edit_last_applied.go Outdated
Copy link
Contributor

@seh seh May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above at line 55 is punctuated, but this comment is not.

Copy link
Contributor Author

@shiywang shiywang May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, nice catch

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2017
@shiywang
Copy link
Contributor Author

shiywang commented May 25, 2017 •
edited
Loading

only add a punctuation
@mengqiy @fabianofranz @liggitt @pwittrock could you guys take a look and give an approval ?
I would like to finish the whole apply subcommand before feature freeze

@shiywang
Copy link
Contributor Author

shiywang commented May 25, 2017

/lgtm

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 25, 2017

@shiywang: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

@mengqiy
Copy link
Member

mengqiy commented May 25, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2017
@liggitt
Copy link
Member

liggitt commented May 25, 2017

/approve

1 similar comment
@pwittrock
Copy link
Member

pwittrock commented May 26, 2017

/approve

@k8s-github-robot
Copy link

k8s-github-robot commented May 26, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mengqiy, pwittrock, shiywang

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

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 k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2017
@mengqiy
Copy link
Member

mengqiy commented May 26, 2017

@k8s-bot pull-kubernetes-node-e2e test this
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@k8s-github-robot
Copy link

k8s-github-robot commented May 26, 2017

Automatic merge from submit-queue (batch tested with PRs 42256, 46479, 45436, 46440, 46417)

@k8s-github-robot k8s-github-robot merged commit f3406bf into kubernetes:master May 26, 2017
@shiywang shiywang deleted the edit branch May 27, 2017 00:17
@dixudx dixudx mentioned this pull request Jul 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

+2 more reviewers

@seh seh seh left review comments

@mengqiy mengqiy mengqiy requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mengqiy mengqiy

@alexandercampbell alexandercampbell

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.