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
Change metadata service helper to work with any query parameters #588
Conversation
7dd08f5
to
34f82d8
Compare
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 |
Hi @davidwtbuxton, It looks like there are two things that aren't passing in the
|
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. |
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. |
34f82d8
to
b23b22e
Compare
d989ec6
to
7f13e73
Compare
There was a problem hiding this 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.
@davidwtbuxton You are also going to need to test with |
ce8874d
to
7b35452
Compare
@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 |
7b35452
to
b02af89
Compare
@davidwtbuxton Tests are failing on |
@davidwtbuxton The |
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.
b02af89
to
80d2a39
Compare
@silvolu is taking this change alright, or should we be waiting for the PR to add a different credential type? |
Yeah we need this to pass scopes to the metadata server, thanks @davidwtbuxton! |
Ha great! I'll review this afternoon pacific time, thanks again! |
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.