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

fix: add with_universe_domain #1408

Merged
merged 4 commits into from
Dec 2, 2023
Merged

fix: add with_universe_domain #1408

merged 4 commits into from
Dec 2, 2023

Conversation

arithmetic1728
Copy link
Contributor

No description provided.

Copy link
Member

@TimurSadykov TimurSadykov left a comment

Choose a reason for hiding this comment

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

No issue with the code.. but let's discuss this on next weekly sync. Maybe we can drop create_with_universe_domain helpers.

@@ -415,6 +415,22 @@ def with_token_uri(self, token_uri):
new_cred._metrics_options = self._metrics_options
return new_cred

def with_universe_domain(self, universe_domain):
Copy link
Member

Choose a reason for hiding this comment

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

not sure if use patters are the same, but for Java I decided not to got with this method. It works best for properties that don't depend on other credential fields, like scopes. Using this could be confusing because it does not make sense change a universe for a particular credential. If it has one - it should be the only one possible. If developer sets up universe programmatically - there should be builder or constructor. And in Java you can emulate same functionality with 1 line (toBuilder().setUniverseDomain().build()). Not sure how things are in Python ) I will add this as a comment to the requirements doc as well, that mentions this method pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with_* is the regular pattern for google-auth lib, for example, with_quota_project_id, with_scopes, with_token_uri etc. Builder/Constructor pattern is specific for Java auth lib, python lib doesn't do this.

Copy link
Member

Choose a reason for hiding this comment

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

Ack, LGTM (passing by)

@arithmetic1728 arithmetic1728 changed the title fix: add with_universe_domain to service account and external cred fix: add with_universe_domain Nov 30, 2023
@arithmetic1728 arithmetic1728 merged commit 505910c into main Dec 2, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants