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

use the upstream construction for kubeconfig evaluation #19335

Merged
merged 5 commits into from
Apr 13, 2018
Merged

use the upstream construction for kubeconfig evaluation #19335

merged 5 commits into from
Apr 13, 2018

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Apr 12, 2018

This changes our kubeconfig construction to use the upstream method with three patches

  1. use --config instead of --kubeconfig for the kubeconfig file
  2. the missing config error references oc login
  3. there is no interactive flow for getting a username and password.

@openshift/sig-master
/assign @soltysh
@mfojtik @liggitt in combination with #19327, you'll be able to use the stock factory construction.

@openshift-ci-robot openshift-ci-robot added sig/master size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2018
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2018
@deads2k deads2k force-pushed the cli-25-configpatch branch 2 times, most recently from 0035066 to 04b10a4 Compare April 12, 2018 20:14
@deads2k
Copy link
Contributor Author

deads2k commented Apr 12, 2018

Interesting that test cmd succeeds and the clusterup install fails

@deads2k
Copy link
Contributor Author

deads2k commented Apr 12, 2018

@pmorie I finally got it in CI!

for: "STDIN": namespaces "kube-dns" is forbidden: caches not synchronized

@deads2k
Copy link
Contributor Author

deads2k commented Apr 12, 2018

/test extended_clusterup

To see if the error is reproducible as I've done it.

soltysh
soltysh suggested changes Apr 13, 2018
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.

I left you some comments. My biggest worries are around 62469, since it's clearly showing it's failing tons of stuff upstream. I'd like to see a green run in upstream and approval there before allowing this PR merge.

pkg/cmd/infra/router/clientcmd.go
@@ -0,0 +1,222 @@
package router
Copy link
Contributor

Choose a reason for hiding this comment

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

This one sucks, a bit. Is there a plan to clean it up later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one sucks, a bit. Is there a plan to clean it up later on?

I was planning on never touching it again.

@knobunc @pravisankar this is highlighting that the router uses a different construction for kubeconfig flags and files than anyone else. It was already like this. This pull just makes it very obvious.

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util/factory_client_access.go
@@ -179,7 +179,11 @@ func DefaultClientConfig(flags *pflag.FlagSet) clientcmd.ClientConfig {
// DEPRECATED: remove and replace with something more accurate
loadingRules.DefaultClientConfig = &clientcmd.DefaultClientConfig

flags.StringVar(&loadingRules.ExplicitPath, "kubeconfig", "", "Path to the kubeconfig file to use for CLI requests.")
if clientcmd.UseOpenShiftKubeConfigValues {
flags.StringVar(&loadingRules.ExplicitPath, clientcmd.OpenShiftKubeConfigFlagName, "", "Path to the kubeconfig file to use for CLI requests.")
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're at it, since as the followup I'll revive the kubeconfig PR I have, maybe we can just squash both changes so that kubeconfig is present always, we'll need it either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While you're at it, since as the followup I'll revive the kubeconfig PR I have, maybe we can just squash both changes so that kubeconfig is present always, we'll need it either way.

I'd rather not extend this pull further if we can avoid it.

@deads2k
Copy link
Contributor Author

deads2k commented Apr 13, 2018

I left you some comments. My biggest worries are around 62469, since it's clearly showing it's failing tons of stuff upstream. I'd like to see a green run in upstream and approval there before allowing this PR merge.

Our needs are different. We have always done this, so it's actually parity for us.

soltysh
soltysh approved these changes Apr 13, 2018
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.

Discussed the 62469 stuff with David on IRC. We have it already covered in https://github.com/openshift/origin/pull/19335/files#diff-5a19ec5edece86a33eb6c3c7a9ddd56aR205
In that case:
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2018
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@deads2k deads2k force-pushed the cli-25-configpatch branch from 04b10a4 to 6f28a10 Compare April 13, 2018 15:59
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2018
@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2018
@deads2k deads2k force-pushed the cli-25-configpatch branch from 6f28a10 to 156c979 Compare April 13, 2018 16:40
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2018
@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

@deads2k
Copy link
Contributor Author

deads2k commented Apr 13, 2018

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit f7d9853 into openshift:master Apr 13, 2018
@soltysh soltysh mentioned this pull request Apr 16, 2018
Closed
mjudeikis pushed a commit to mjudeikis/origin that referenced this pull request Apr 17, 2018
@openshift-merge-robot @mjudeikis
Merge pull request openshift#19335 from deads2k/cli-25-configpatch
0caf177 
use the upstream construction for kubeconfig evaluation
@deads2k deads2k deleted the cli-25-configpatch branch July 3, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

@bparees bparees Awaiting requested review from bparees

@sttts sttts Awaiting requested review from sttts

Assignees

@soltysh soltysh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. sig/master size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@deads2k @openshift-bot @openshift-ci-robot @soltysh @openshift-merge-robot