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

fixing flipped sign in expiry time padding #55

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

flylo
Copy link
Contributor

@flylo flylo commented Mar 30, 2018

We ran into a bug where any job that runs long enough for the API token to expire will fail with a 401 Unauthorized ApiException. We found that this was due to a flipped sign in the padding added to the expiry. I've updated the _is_expired method to return True if the token expires in 5 minutes, as opposed to the current logic which returns True if the token expired 5 minutes ago. I've also added an extra assertion to ensure that the _load_gcp_token method actually updates the expiry timestamp.

Please let me know if you have any questions or comments!

@flylo
Copy link
Contributor Author

flylo commented Mar 30, 2018

also FYI this stems from a discussion in:
kubernetes-client/python#492

@flylo
Copy link
Contributor Author

flylo commented Apr 2, 2018

anyone have any idea what's wrong with my py2.7 pep8? it doesn't say what the actual failure is.

@flylo
Copy link
Contributor Author

flylo commented Apr 2, 2018

cc @mbohlool who seems to be the one who worked on the expiry time logic

@yliaog
Copy link
Contributor

yliaog commented Apr 4, 2018

@flylo I reran the CI, and it is still failing https://travis-ci.org/kubernetes-client/python-base/jobs/361191252

in particular, i see the following error message.
/tmp/tmp.ztIlFmpO08/python/kubernetes/config/kube_config_test.py:215:80: E501 line too long (80 > 79 characters)

@flylo
Copy link
Contributor Author

flylo commented Apr 5, 2018

@yliaog still won't pass and all I can see are a bunch of user warnings to use pycodestyle instead of pep8. what exactly does this mean?
ERROR: InvocationError for command '/tmp/tmp.mcnTCgxokv/python/scripts/update-pep8.sh' (exited with code 1)

@flylo
Copy link
Contributor Author

flylo commented Apr 5, 2018

@yliaog what is the purpose of this script here? Why does my build fail because some env is set or my git status is clean?

@yliaog
Copy link
Contributor

yliaog commented Apr 5, 2018

I'm not quite sure, I can take a look tomorrow.

https://github.com/kubernetes-client/python/blame/master/scripts/update-pep8.sh#L80 shows @dims added it in kubernetes-client/python@5c82309

@codecov-io
Copy link

codecov-io commented Apr 5, 2018

Codecov Report

Merging #55 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   93.29%   93.36%   +0.07%     
==========================================
  Files          11       11              
  Lines         939      949      +10     
==========================================
+ Hits          876      886      +10     
  Misses         63       63
Impacted Files Coverage Δ
config/kube_config.py 89.69% <100%> (ø) ⬆️
config/kube_config_test.py 93.31% <100%> (+0.2%) ⬆️

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 2010e2d...3932d29. Read the comment docs.

@flylo
Copy link
Contributor Author

flylo commented Apr 5, 2018

Figured out that the diff is between the PR files and the pep8 auto-formatted files. There should probably be some log messages in that script as it wasn't clear to me what that was doing until I went back to the PR where it was merged.
@yliaog this good to merge now?

@yliaog
Copy link
Contributor

yliaog commented Apr 5, 2018

Yes, thanks for fixing it.

@yliaog yliaog merged commit 789de6a into kubernetes-client:master Apr 5, 2018
@yliaog yliaog self-assigned this Apr 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants