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

Change metadata service helper to work with any query parameters #588

Conversation

davidwtbuxton
Copy link
Contributor

Part of #579

This helper is used with '?recursive=true' in one place, and can now be used by
IDTokenCredentials for requests with query parameters to the metadata identity
end-point.

This change will allow making requests to the token end-point with '?scopes=..'
query parameters.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 9, 2020
@davidwtbuxton davidwtbuxton force-pushed the feature/579-app-engine-scopes branch 2 times, most recently from 7dd08f5 to 34f82d8 Compare August 9, 2020 20:57
@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 17, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 17, 2020
@busunkim96 busunkim96 added the automerge Merge the pull request once unit tests and other checks pass. label Aug 17, 2020
@gcf-merge-on-green
Copy link

Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR

@busunkim96
Copy link
Contributor

Hi @davidwtbuxton,

It looks like there are two things that aren't passing in the Kokoro job:

  • cover
    .nox/cover/lib/python3.7/site-packages/google/auth/compute_engine/_metadata.py        70      0     16      1    99%   
    142->145
    
  • lint
    Install nox pip install nox and run the autoformat session nox -s blacken. If this is troublesome I can also push to yoru branch.
    black --check google tests noxfile.py setup.py docs/conf.py
    would reformat /tmpfs/src/github/google-auth-library-python/google/auth/compute_engine/_metadata.py
    would reformat /tmpfs/src/github/google-auth-library-python/google/auth/compute_engine/credentials.py
    All done! 💥 💔 💥
    2 files would be reformatted, 70 files would be left unchanged.
    

@gcf-merge-on-green
Copy link

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 17, 2020
@davidwtbuxton
Copy link
Contributor Author

Nuts. I don't understand why that branch isn't covered by the tests. I will run black to satisfy the linter, and dig deeper on the coverage issue.

@davidwtbuxton davidwtbuxton requested a review from a team as a code owner October 18, 2020 15:38
@davidwtbuxton davidwtbuxton force-pushed the feature/579-app-engine-scopes branch 2 times, most recently from d989ec6 to 7f13e73 Compare October 18, 2020 16:21
@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 21, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 21, 2020
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

Please ensure that unit tests pass on all supported versions of Python. The best path is to run nox, after ensuring that python3.6, python3.7, and python3.8 are on your path.

google/auth/compute_engine/_metadata.py Outdated Show resolved Hide resolved
@tseaver
Copy link
Contributor

tseaver commented Oct 21, 2020

@davidwtbuxton You are also going to need to test with python2.7 and python3.5', exercised via nox -s unit_prev_versions`.

@davidwtbuxton davidwtbuxton force-pushed the feature/579-app-engine-scopes branch 2 times, most recently from ce8874d to 7b35452 Compare October 22, 2020 07:20
@davidwtbuxton
Copy link
Contributor Author

@tseaver I've updated the code with your suggestion for simplifying the params logic, thank you.

Kokoro failed because of the unrelated changes in commit 7e15258 , and tests are also broken in master. I've rebased changes in this branch on the top of release v1.21.3 (from 23 September 2020) and all tests pass now, but of course this feature/579-app-engine-scopes branch is out-of-date with the head of master.

The unit_prev_versions session isn't in noxfile, but I did run nox -s unit with Python 2.7 and Python 3.5 -> 3.8, all tests passed.

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 22, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 22, 2020
@tseaver
Copy link
Contributor

tseaver commented Oct 22, 2020

@davidwtbuxton Tests are failing on master for unit-3.6 and unit-3.7 (#629), due to #629.

@tseaver
Copy link
Contributor

tseaver commented Oct 22, 2020

@davidwtbuxton The unit_prev_versions session is definitely present on master.

This helper is used with '?recursive=true' in one place, and can now be used by
IDTokenCredentials for requests with query parameters to the metadata identity
end-point.

This change will allow making requests to the token end-point with '?scopes=..'
query parameters.
@davidwtbuxton
Copy link
Contributor Author

@tseaver I've updated this branch with the latest changes from master, including the fixes from #630. Thank you, David

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 23, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 23, 2020
@tseaver tseaver added the automerge Merge the pull request once unit tests and other checks pass. label Oct 23, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 5906c85 into googleapis:master Oct 23, 2020
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 23, 2020
@busunkim96
Copy link
Contributor

@silvolu is taking this change alright, or should we be waiting for the PR to add a different credential type?

@silvolu
Copy link

silvolu commented Oct 26, 2020

Yeah we need this to pass scopes to the metadata server, thanks @davidwtbuxton!
I'm working on a PR to change Compute Credentials to accept user defined scopes, we decided to be consistent with the existing implementation on Go and Node where we allow developers to specify scopes, and then depending on where they are running they get a token with the scopes they requested (e.g. GKE) or with the predefined scopes (e.g. GCE).

@davidwtbuxton
Copy link
Contributor Author

@silvolu sounds like #633

@silvolu
Copy link

silvolu commented Oct 26, 2020

Ha great! I'll review this afternoon pacific time, thanks again!

@davidwtbuxton davidwtbuxton deleted the feature/579-app-engine-scopes branch October 30, 2020 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants