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

kubectl default container behavior #99833

Merged

Conversation

mengjiao-liu
Copy link
Member

@mengjiao-liu mengjiao-liu commented Mar 5, 2021

What type of PR is this?

/kind feature

What this PR does / why we need it:

specify the default container for kubectl attach, cp, exec command with annotation kubectl.kubernetes.io/default-container
The following files are created
staging/src/k8s.io/kubectl/pkg/cmd/util/default-container.go
This avoids duplicate code and help having a single place for maintenance.

more details can be seen in the KEP KEP-2227: default container behavior

Which issue(s) this PR fixes:

Ref enhancements issue

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Users might specify the `kubectl.kubernetes.io/default-container` annotation in a Pod to preselect container for kubectl commands.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP] https://github.com/kubernetes/enhancements/blob/master/keps/sig-cli/2227-kubectl-default-container/README.md

@k8s-ci-robot k8s-ci-robot added release-note kind/feature size/S cncf-cla: yes do-not-merge/needs-sig needs-triage needs-priority needs-ok-to-test labels Mar 5, 2021
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 5, 2021

Hi @mengjiao-liu. 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 and removed do-not-merge/needs-sig labels Mar 5, 2021
@k8s-ci-robot k8s-ci-robot requested review from dixudx and dougsland Mar 5, 2021
@mengjiao-liu mengjiao-liu force-pushed the kubectl-default-container-attach branch 3 times, most recently from 31cfe9b to fe5dd12 Compare Mar 5, 2021
@mengjiao-liu mengjiao-liu changed the title kubectl attach command default container behavior with annotation kubectl.kubernetes.io/default-container kubectl attach command default container behavior Mar 5, 2021
@dougsland
Copy link
Member

@dougsland dougsland commented Mar 5, 2021

On queue for the review.
/cc @pacoxu

@k8s-ci-robot k8s-ci-robot requested a review from pacoxu Mar 5, 2021
pacoxu
pacoxu approved these changes Mar 5, 2021
Copy link
Member

@pacoxu pacoxu left a comment

/lgtm
👍

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 5, 2021
@pacoxu
Copy link
Member

@pacoxu pacoxu commented Mar 5, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 5, 2021
@dougsland
Copy link
Member

@dougsland dougsland commented Mar 5, 2021

@mengjiao-liu looks like there is a conflict. Could you please rebase so we can do a fresh review?

@dougsland
Copy link
Member

@dougsland dougsland commented Mar 5, 2021

/cc @eddiezane

@k8s-ci-robot k8s-ci-robot requested a review from eddiezane Mar 5, 2021
@k8s-ci-robot k8s-ci-robot added size/M and removed lgtm size/S labels Mar 8, 2021
@mengjiao-liu mengjiao-liu requested a review from dougsland Mar 8, 2021
@mengjiao-liu mengjiao-liu force-pushed the kubectl-default-container-attach branch from 722cb94 to 02b5717 Compare Mar 8, 2021
@mengjiao-liu mengjiao-liu changed the title kubectl attach and cp command default container behavior kubectl default container behavior Mar 8, 2021
@pacoxu
Copy link
Member

@pacoxu pacoxu commented Mar 9, 2021

/lgtm
@dougsland

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 9, 2021
@dougsland
Copy link
Member

@dougsland dougsland commented Mar 9, 2021

/lgtm
Thanks @mengjiao-liu @pacoxu

@annajung
Copy link
Member

@annajung annajung commented Mar 9, 2021

Hi @mengjiao-liu, a friendly reminder that the code freeze for 1.21 is today.
Please make sure to get the necessary approval for this to merge by EOD PST.

Copy link
Member

@dougsland dougsland left a comment

/lgtm

@mengjiao-liu mengjiao-liu force-pushed the kubectl-default-container-attach branch from 02b5717 to 88e5301 Compare Mar 10, 2021
@k8s-ci-robot k8s-ci-robot added size/S and removed lgtm size/M labels Mar 10, 2021
@eddiezane
Copy link
Member

@eddiezane eddiezane commented Mar 10, 2021

/retest
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 10, 2021
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eddiezane, mengjiao-liu, pacoxu

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 label Mar 10, 2021
@palnabarun
Copy link
Member

@palnabarun palnabarun commented Mar 10, 2021

Adding the PR to the v1.21 milestone, since most of the changes were lgtm'ed prior to Code Freeze and the change here doesn't carry much risk.

/milestone v1.21

@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 10, 2021
@pacoxu
Copy link
Member

@pacoxu pacoxu commented Mar 10, 2021

/retest

@k8s-ci-robot k8s-ci-robot merged commit 9132f87 into kubernetes:master Mar 10, 2021
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/kubectl cncf-cla: yes kind/feature lgtm ok-to-test priority/important-soon release-note sig/cli size/S triage/accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants