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

feat: add quota project to base credentials class #546

Merged
merged 11 commits into from Jul 9, 2020

Conversation

busunkim96
Copy link
Contributor

@busunkim96 busunkim96 commented Jun 24, 2020

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 a quota_project_id to be passed.

import google.auth

credentials, project = google.auth.default(quota_project_id='my-project')
import google.auth

credentials, project = google.auth.load_credentials_from_file(
    'service_account.json',
    quota_project_id='my-project'
)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 24, 2020
@busunkim96 busunkim96 added the automerge Merge the pull request once unit tests and other checks pass. label Jul 6, 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

@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 Jul 7, 2020
@busunkim96 busunkim96 added the automerge Merge the pull request once unit tests and other checks pass. label Jul 9, 2020
@busunkim96 busunkim96 merged commit 3dda7b2 into master Jul 9, 2020
@busunkim96 busunkim96 deleted the more-quota-project branch July 9, 2020 17:39
@hiranya911
Copy link
Contributor

hiranya911 commented Jul 13, 2020

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 firebase-admin where we extend this interface.

firebase_admin/db.py:983:0: W0223: Method 'with_quota_project' is abstract in class 'Credentials' but is not overridden (abstract-method)

@hiranya911
Copy link
Contributor

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.

@busunkim96
Copy link
Contributor Author

@hiranya911

Hmm, maybe it makes more sense to have the function raise ValueError?

    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)
Copy link

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)

Copy link
Contributor Author

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!

@hiranya911
Copy link
Contributor

@busunkim96 thanks for looking into it. Yes, perhaps the default implementation in the abstract class should raise ValueError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the pull request once unit tests and other checks pass. 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