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

[AKS] rename admin k8s context so it won't overwite user context #6529

Merged
merged 3 commits into from
Jun 12, 2018

Conversation

mboersma
Copy link
Member

@mboersma mboersma commented Jun 7, 2018

Fixes an issue where az aks get-credentials didn't distinguish between the admin and user versions when merging into ~/.kube/config.

Before:

$ rm -f ~/.kube/config
$ az aks get-credentials -g aztest -n aztest
Merged "aztest" as current context in /Users/matt/.kube/config
$ az aks get-credentials -g aztest -n aztest --admin
Merged "aztest" as current context in /Users/matt/.kube/config
$ kubectl config get-contexts
CURRENT   NAME      CLUSTER   AUTHINFO                     NAMESPACE
*         aztest    aztest    clusterAdmin_aztest_aztest

After:

$ git checkout rename-admin-k8s-config
Switched to branch 'rename-admin-k8s-config'
$ rm -f ~/.kube/config
$ az aks get-credentials -g aztest -n aztest
Merged "aztest" as current context in /Users/matt/.kube/config
$ az aks get-credentials -g aztest -n aztest --admin
Merged "aztest-admin" as current context in /Users/matt/.kube/config
$ kubectl config get-contexts
CURRENT   NAME           CLUSTER   AUTHINFO                     NAMESPACE
          aztest         aztest    clusterUser_aztest_aztest    
*         aztest-admin   aztest    clusterAdmin_aztest_aztest 

cc: @slack


  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

@codecov-io
Copy link

codecov-io commented Jun 8, 2018

Codecov Report

Merging #6529 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##           dev   #6529   +/-   ##
===================================
  Coverage    0%      0%           
===================================
  Files       11      11           
  Lines      133     133           
  Branches     9       9           
===================================
  Misses     133     133

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ce769a...a463c37. Read the comment docs.

@mboersma
Copy link
Member Author

mboersma commented Jun 8, 2018

Given #6497 I assume I'll have to update HISTORY.rst in this PR before it can be merged.

admin_name = ctx['name'] + '-admin'
addition['current-context'] = ctx['name'] = admin_name
break
except (KeyError, TypeError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the try catch in the loop instead of out of the loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed.

@mboersma mboersma force-pushed the rename-admin-k8s-config branch from 9ee9439 to a463c37 Compare June 12, 2018 16:14
@mboersma
Copy link
Member Author

Rebased to fix merge conflict.

@troydai
Copy link
Contributor

troydai commented Jun 12, 2018

Ok. I'll merge it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants