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.

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

Remove kubectl exec deprecated -p/--pod flag #76713

Merged
merged 1 commit into from
Jun 1, 2019
Merged

Remove kubectl exec deprecated -p/--pod flag #76713

merged 1 commit into from
Jun 1, 2019

Conversation

prksu
Copy link
Contributor

@prksu prksu commented Apr 17, 2019 •
edited
Loading

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

--pod/-p flag was marked as deprecated flag since #66558 now it's enough time to remove it after marking the flag as deprecated.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

ping @soltysh

Does this PR introduce a user-facing change?:

Remove deprecated --pod/-p flag from kubectl exec. The flag has been marked as deprecated since k8s version v1.12

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 17, 2019
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 17, 2019
@prksu
Copy link
Contributor Author

prksu commented Apr 17, 2019

/assign @soltysh

neolit123
neolit123 reviewed Apr 17, 2019
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/priority important-longterm
/ok-to-test

@prksu
it would be best if you explain in the release note that the flag was deprecated since k8s version x.y.
thanks.

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 17, 2019
@prksu
Copy link
Contributor Author

prksu commented Apr 17, 2019

@neolit123
updated. thanks

@prksu prksu force-pushed the remove-exec-deprecated-pod-flag branch from 23182fa to bf75d50 Compare April 27, 2019 17:14
@prksu
Copy link
Contributor Author

prksu commented May 9, 2019

/cc @soltysh need an approval

@k8s-ci-robot
Copy link
Contributor

@prksu: GitHub didn't allow me to request PR reviews from the following users: an, approval, need.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @soltysh need an approval

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.

@k8s-ci-robot k8s-ci-robot requested a review from soltysh May 9, 2019 13:13
@prksu prksu mentioned this pull request May 17, 2019
Closed
6 tasks
@prksu
Copy link
Contributor Author

prksu commented May 22, 2019

/cc @Kargakis

0xmichalis
0xmichalis suggested changes May 22, 2019
View reviewed changes
Copy link
Contributor

@0xmichalis 0xmichalis left a comment

Choose a reason for hiding this comment

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

Some code paths in Complete, Validate, and Run can be simplified as a result of this removal.

@prksu prksu force-pushed the remove-exec-deprecated-pod-flag branch from bf75d50 to a59b476 Compare May 23, 2019 13:44
@prksu
Copy link
Contributor Author

prksu commented May 23, 2019

Some code paths in Complete, Validate, and Run can be simplified as a result of this removal.

@Kargakis

the p.PodName is still used in kubectl cp

https://github.com/kubernetes/kubernetes/blob/bf75d500f5edd2e3e6ed091c0fc3412ab78132c4/pkg/kubectl/cmd/exec/exec.go#L223-L225

and

https://github.com/kubernetes/kubernetes/blob/bf75d500f5edd2e3e6ed091c0fc3412ab78132c4/pkg/kubectl/cmd/exec/exec.go#L290-L297

maybe we need change legacy pod getter in kubectl cp to use resource.Builder first, so we can remove unused code above.

@prksu
Copy link
Contributor Author

prksu commented May 23, 2019

/retest

@0xmichalis
Copy link
Contributor

@prksu hrm, now I see. Don't feel strongly in any way, leaving this up to @soltysh.

@prksu
Copy link
Contributor Author

prksu commented May 23, 2019

/retest

@prksu
Copy link
Contributor Author

prksu commented May 23, 2019

ping @soltysh

soltysh
soltysh approved these changes May 30, 2019
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2019
@fejta-bot
Copy link

/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 or /hold comment for consistent failures.

@prksu
Copy link
Contributor Author

prksu commented May 31, 2019

/retest

@fejta-bot
Copy link

/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 or /hold comment for consistent failures.

@claurence
Copy link

/milestone v1.15

@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone May 31, 2019
@prksu
Copy link
Contributor Author

prksu commented May 31, 2019

/retest

@prksu
Copy link
Contributor Author

prksu commented May 31, 2019

/test pull-kubernetes-bazel-build

@prksu
Copy link
Contributor Author

prksu commented Jun 1, 2019

/retest

@fejta-bot
Copy link

/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 or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 48f2be9 into kubernetes:master Jun 1, 2019
@prksu
Copy link
Contributor Author

prksu commented Jun 1, 2019 •
edited
Loading

@soltysh @Kargakis @claurence thanks

@prksu prksu deleted the remove-exec-deprecated-pod-flag branch June 4, 2019 01:32
@atiratree atiratree mentioned this pull request Jan 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@0xmichalis 0xmichalis 0xmichalis requested changes

@soltysh soltysh soltysh approved these changes

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@eparis eparis Awaiting requested review from eparis

Assignees

@soltysh soltysh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.15
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@prksu @k8s-ci-robot @0xmichalis @fejta-bot @claurence @neolit123 @soltysh