Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

Add option to refresh gcp token when config is cmd-path #175

Merged
merged 1 commit into from
Jan 8, 2020

Conversation

jfrabaute
Copy link
Contributor

The python client does not deal with "gcloud" gke auth in the kube config file as it's based on the "cmd-path" auth.

This is adding the code to take care of this auth.
This should solve kubernetes-client/python#754

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2019
@jfrabaute
Copy link
Contributor Author

Looks like the test is failing on python style code problems on unrelated files (not thouched in that review).
I fixed a few, but I have one that I don't know about:

/tmp/tmp.0CpklmniPV/python/kubernetes/utils/quantity.py:24:80: E501 line too long (91 > 79 characters)

@roycaihw
Copy link
Member

/assign @yliaog

@micw523
Copy link
Contributor

micw523 commented Oct 29, 2019

The CI is failing on the files you modified. Take a look at the diff output - or to save time, just use autopep8 --aggressive --aggressive --in-place kube_config.py.

E501 is ignored in judging the success of the CI task.

@jfrabaute
Copy link
Contributor Author

The CI is failing on the files you modified. Take a look at the diff output - or to save time, just use autopep8 --aggressive --aggressive --in-place kube_config.py.

Oops, yes. Fixed, build seems to pass now.

@jfrabaute
Copy link
Contributor Author

Hi,

Any chance to have this reviewed?

config/kube_config.py Show resolved Hide resolved
@@ -133,6 +135,47 @@ def as_data(self):
return self._data


class CommandTokenSource(object):
def __init__(self, cmd, args, tokenKey, expiryKey):

Copy link
Contributor

Choose a reason for hiding this comment

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

why the extra blank line here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed (when PR will be updated)

config/kube_config_test.py Show resolved Hide resolved
@@ -1439,7 +1444,8 @@ def test_list_kube_config_contexts(self):
{'context': {'cluster': 'ssl', 'user': 'ssl'}, 'name': 'ssl'},
{'context': {'cluster': 'default', 'user': 'simple_token'},
'name': 'simple_token'},
{'context': {'cluster': 'default', 'user': 'expired_oidc'}, 'name': 'expired_oidc'}]
{'context': {'cluster': 'default', 'user': 'expired_oidc'},
'name': 'expired_oidc'}]
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a test case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests and updated the PR.

@codecov-io
Copy link

codecov-io commented Jan 7, 2020

Codecov Report

Merging #175 into master will decrease coverage by 0.94%.
The diff coverage is 74.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
- Coverage   93.41%   92.46%   -0.95%     
==========================================
  Files          13       13              
  Lines        1398     1474      +76     
==========================================
+ Hits         1306     1363      +57     
- Misses         92      111      +19
Impacted Files Coverage Δ
config/kube_config_test.py 95.56% <100%> (+0.27%) ⬆️
config/kube_config.py 84.25% <57.44%> (-3.28%) ⬇️
config/dateutil.py 97.91% <0%> (+2.08%) ⬆️

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 a2d1024...39113de. Read the comment docs.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 8, 2020
@jfrabaute jfrabaute force-pushed the cmdpath branch 3 times, most recently from ebb2bc7 to eeb8b54 Compare January 8, 2020 05:10
config/kube_config_test.py Outdated Show resolved Hide resolved
config/kube_config_test.py Outdated Show resolved Hide resolved
config/kube_config_test.py Outdated Show resolved Hide resolved
config/kube_config_test.py Outdated Show resolved Hide resolved
@jfrabaute jfrabaute force-pushed the cmdpath branch 2 times, most recently from 0ee3cb5 to d3bc0a9 Compare January 8, 2020 19:17
@yliaog
Copy link
Contributor

yliaog commented Jan 8, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jfrabaute, yliaog

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 Jan 8, 2020
@k8s-ci-robot k8s-ci-robot merged commit 7ea5cb4 into kubernetes-client:master Jan 8, 2020
@jfrabaute
Copy link
Contributor Author

Thanks @yliaog for the reviews.
I'll work on the other PR and add tests now: #169

@palnabarun
Copy link
Member

Note to self: We should add documentation for the functionality implemented here.

/cc

cc: @roycaihw

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants