Skip to content

fix kubectl --local flag bug across all commands #93298

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
Closed
wants to merge 1 commit into from
Closed

fix kubectl --local flag bug across all commands #93298

wants to merge 1 commit into from

Conversation

SaiHarshaK
Copy link
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
Kubectl "local" flag won't ask for kubeconfig after deprecated functionality in #90243 is removed.

Which issue(s) this PR fixes:
Fixes #93188

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. labels Jul 21, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @SaiHarshaK. 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/kubectl area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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 Jul 21, 2020
@fedebongio
Copy link
Contributor

/assign @deads2k @soltysh

deads2k
deads2k reviewed Jul 21, 2020
View reviewed changes
staging/src/k8s.io/client-go/tools/clientcmd/client_config.go Outdated
// DefaultClientConfig represents the legacy behavior of this package for defaulting
// DEPRECATED will be replace
DefaultClientConfig = DirectClientConfig{*clientcmdapi.NewConfig(), "", &ConfigOverrides{
ClusterDefaults: ClusterDefaults,
}, nil, NewDefaultClientConfigLoadingRules(), promptedCredentials{}}
)

// getDefaultServer returns a default setting for DefaultClientConfig
// DEPRECATED
func getDefaultServer() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

pull this bit out of your PR please. That will contain it sig-cli.

Copy link
Contributor Author

@SaiHarshaK SaiHarshaK Jul 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'll revert the changes to this file and relevant ones

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ill squash the commits?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SaiHarshaK
To complete the pull request process, please assign deads2k
You can assign the PR to them by writing /assign @deads2k in a comment when ready.

The full list of commands accepted by this bot can be found 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

@SaiHarshaK SaiHarshaK force-pushed the fix_local_flag branch 2 times, most recently from 6e88616 to 3be6384 Compare July 22, 2020 10:06
@SaiHarshaK SaiHarshaK requested a review from deads2k July 22, 2020 18:44
@deads2k
Copy link
Contributor

deads2k commented Jul 22, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 22, 2020
@SaiHarshaK
Copy link
Contributor Author

/retest

soltysh
soltysh requested changes Jul 26, 2020
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.

/hold

pkg/kubectl/cmd/convert/convert.go
o.Namespace, _, err = f.ToRawKubeConfigLoader().Namespace()
if err != nil {
return err
if !o.local {
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this solves the problem, that's not the right approach. See what I wrote in #93188, towards the bottom. You picked the hacky solution which we don't want to happen. The entire sig including the tech leads would like to see a more general solution, rather this one off.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2020
@soltysh
Copy link
Contributor

soltysh commented Aug 26, 2020

/close
Let's discuss possible approaches in #93188, and only open PRs once we have a general consensus about it.

@k8s-ci-robot
Copy link
Contributor

@soltysh: Closed this PR.

In response to this:

/close
Let's discuss possible approaches in #93188, and only open PRs once we have a general consensus about it.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh requested changes

@lavalamp lavalamp Awaiting requested review from lavalamp

@shiywang shiywang Awaiting requested review from shiywang

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@soltysh soltysh

@deads2k deads2k

Labels
area/kubectl area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubectl --local is not quite local across all the commands
5 participants
@SaiHarshaK @k8s-ci-robot @fedebongio @deads2k @soltysh