Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.

fix: add with_universe_domain#1408

Merged
arithmetic1728 merged 4 commits intomainfrom
ud
Dec 2, 2023
Merged

fix: add with_universe_domain#1408
arithmetic1728 merged 4 commits intomainfrom
ud

Conversation

@arithmetic1728
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown

@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.

new_cred._metrics_options = self._metrics_options
return new_cred

def with_universe_domain(self, universe_domain):
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants