-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: add quota project to base credentials class #546
Conversation
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 |
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. |
Isn't this a breaking change? Should it have been released as 1.19.0? We are already seeing test failures in downstream libs like
|
Actually this seems to be because pylint considers any method in an abstract class that throws NotImplementedError to be abstract: http://pylint-messages.wikidot.com/messages:w0223 Not quite sure if that should be considered a breaking change, or write it off as pylint being too pedantic. |
Hmm, maybe it makes more sense to have the function raise def with_quota_project(self, quota_project_id):
raise ValueError("Credentials don't support quota project.") |
@@ -114,7 +116,7 @@ def load_credentials_from_file(filename, scopes=None): | |||
try: | |||
credentials = credentials.Credentials.from_authorized_user_info( | |||
info, scopes=scopes | |||
) | |||
).with_quota_project(quota_project_id) |
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.
Can not find anywhere that load_credentials_from_file
is called with a quota_project_id
parameter.
So quota_project_id
seems always be the default None
.
At the same time, quota_project_id
loaded in info
(from json file) could have work well but overwrote by the .with_quota_project(quota_project_id)
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.
Ah yes, it looks like I introduced a bug. Will open a PR shortly. Thanks for the review!
@busunkim96 thanks for looking into it. Yes, perhaps the default implementation in the abstract class should raise |
Make
quota_project_id
a property of the base credentials class and add it to the existing credentials types.Note that I didn't make
with_quota_project
an abstract method on the base credentials class because that would be a breaking change.This PR also modifies functions in
default
to allow aquota_project_id
to be passed.