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

refactor: split 'with_quota_project' into separate base class #561

Merged
merged 3 commits into from
Sep 2, 2020

Conversation

busunkim96
Copy link
Contributor

#546 (comment)

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)

#546 (comment)

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.

CC @hiranya911

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 15, 2020
@tseaver
Copy link
Contributor

tseaver commented Jul 21, 2020

FWIW, the NotImplementedError in the abstract base class could be considered "correct", as it adds a contract that all concrete subclasses must implement. For better backward-compatibility, it might be better to add another abstract class with that method, rather than changing it to raise ValueError. E.g.:

class CredentialsWithQuotaProject(Credentials):
    @abc.abstractmethod
    def with_quota_project(self, quota_project_id):
        """Returns a copy of these credentials with a modified quota project
        Args:
            quota_project_id (str): The project to use for quota and
                billing purposes
        Returns:
            google.oauth2.credentials.Credentials: A new credentials instance.
        """
        raise NotImplementedError("This class does not support quota project.")

@busunkim96 busunkim96 closed this Jul 31, 2020
@busunkim96 busunkim96 reopened this Jul 31, 2020
@hiranya911
Copy link
Contributor

Any plans to implement the change suggested by @tseaver? This is currently causing a failure in our CI system. I'm thinking of adding a lint ignore directive for now.

@google-cla
Copy link

google-cla bot commented Aug 27, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 27, 2020
@tseaver
Copy link
Contributor

tseaver commented Aug 27, 2020

@google-cla I consent.

@tseaver tseaver added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 27, 2020
@tseaver tseaver force-pushed the quota-project-value-error branch from 702cb2f to 053d3b5 Compare August 27, 2020 16:35
@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 27, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 27, 2020
@google-cla
Copy link

google-cla bot commented Aug 27, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 27, 2020
@tseaver
Copy link
Contributor

tseaver commented Aug 27, 2020

@google-cla I consent

@busunkim96 I've pushed dc3ae79 with my suggested refactoring. I don't know why the CLA bot is unhappy with that commit. If you don't like how that looks, feel free to rebase my commit away.

@tseaver
Copy link
Contributor

tseaver commented Aug 27, 2020

@googlebot I consent.

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 27, 2020
@busunkim96
Copy link
Contributor Author

@tseaver Thank you! 🙏

@tseaver tseaver changed the title fix: make with_quota_project raise ValueError instead of NotImplementedError refactor: split 'with_quota_project' into separate base class Sep 2, 2020
@tseaver tseaver merged commit 41599ae into master Sep 2, 2020
@tseaver tseaver deleted the quota-project-value-error branch September 2, 2020 18:55
busunkim96 pushed a commit that referenced this pull request Sep 10, 2020
* refactor: split 'with_quota_project' into separate base class (#561)
* feat: implements the OAuth token exchange spec based on rfc8693
busunkim96 added a commit that referenced this pull request Oct 22, 2020
* refactor: split 'with_quota_project' into separate base class (#561)

Co-authored-by: Tres Seaver <tseaver@palladion.com>

* fix: dummy commit to trigger a auto release (#597)

* chore: release 1.21.1 (#599)

* chore: updated CHANGELOG.md [ci skip]

* chore: updated setup.cfg [ci skip]

* chore: updated setup.py

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: migrate signBlob to iamcredentials.googleapis.com (#600)

Migrate signBlob from iam.googleapis.com to iamcredentials.googleapis.com.

This API is deprecated and will be shutdown in one year.

This is used google.auth.iam.Signer.
Added a system_test to sanity check the implementation.

* chore: release 1.21.2 (#601)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: fix expiry for `to_json()` (#589)

* This patch for </issues/501> includes the following fixes:

- The access token is always set to `None`, so the fix involves using (the access) `token` from the saved JSON credentials file.
- For refresh needs, `expiry` also needs to be saved via `to_json()`.
    - DUMP: As `expiry` is a `datetime.datetime` object, serialize to `datetime.isoformat()` in the same [`oauth2client` format](https://github.com/googleapis/oauth2client/blob/master/oauth2client/client.py#L55) for consistency.
    - LOAD: Add code to restore `expiry` back to `datetime.datetime` object when imported.
    - LOAD: If `expiry` was unsaved, automatically set it as expired so refresh takes place.
- Minor `scopes` updates
    - DUMP: Add property for `scopes` so `to_json()` can grab it
    - LOAD: `scopes` may be saved as a string instead of a JSON array (Python list), so ensure it is Sequence[str] when imported.

* chore: add default CODEOWNERS (#609)

* chore: release 1.21.3 (#607)

* feat: add asyncio based auth flow (#612)

* feat: asyncio http request logic and asynchronous credentials logic  (#572)

Co-authored-by: Anirudh Baddepudi <43104821+anibadde@users.noreply.github.com>

* chore: release 1.22.0 (#615)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: move aiohttp to extra as it is currently internal surface (#619)

Fix #618. Removes aiohttp from required dependencies to lessen dependency tree for google-auth.

This will need to be looked at again as more folks use aiohttp and once the surfaces goes to public visibility.

* chore: release 1.22.1 (#620)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: remove checks for ancient versions of Cryptography (#596)

Refs #595 (comment) 

I see no point in checking whether someone is running a version of https://github.com/pyca/cryptography/ from 2014 that doesn't even compile against modern versions of OpenSSL anymore.

* chore: sync to master

Syncs to master.
Fixes broken unit tests in Python 3.6 and 3.7.
Aligns test_identity_pool.py with test_aws.py.

Co-authored-by: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>
Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: arithmetic1728 <58957152+arithmetic1728@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: wesley chun <wescpy@gmail.com>
Co-authored-by: Christopher Wilcox <crwilcox@google.com>
Co-authored-by: Anirudh Baddepudi <43104821+anibadde@users.noreply.github.com>
Co-authored-by: Aarni Koskela <akx@iki.fi>
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.

5 participants