NotificationsYou must be signed in to change notification settings
Fork321
Star786

feat: support data_governance_type#1708

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Linchin merged 4 commits intogoogleapis:mainfromLinchin:data-gov-2

Nov 1, 2023

Merged

feat: support data_governance_type#1708

Linchin merged 4 commits intogoogleapis:mainfromLinchin:data-gov-2

Nov 1, 2023

Conversation

Copy link

Contributor

Linchin commentedOct 30, 2023•
edited
Loading

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as abug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes#1707 🦕

feat: support data_governance_type

b703f0c

Linchin requested review froma team ascode owners

October 30, 2023 23:35

Linchin requested a review fromprash-mi

October 30, 2023 23:35

product-auto-labelbot added size: m

Pull request size is medium.

api: bigqueryIssues related to the googleapis/python-bigquery API. labels

Oct 30, 2023

Linchin requested review fromchalmerlowe andshollyman and removed request forprash-mi

October 30, 2023 23:35

Linchin added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Oct 30, 2023

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Oct 30, 2023

shollyman reviewed

Nov 1, 2023

View reviewed changes

Copy link

Contributor

shollyman left a comment

Choose a reason for hiding this comment

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

Is it worth adding the new field to an existing system/integration test related to routines, or are there extensive dependencies here to properly setup masking?

google/cloud/bigquery/routine/routine.py Outdated


		@data_governance_type.setter
		defdata_governance_type(self,value):
		ifvalueisnotNoneandvaluenotinself._DATA_GOVERNANCE_TYPE:

Copy link

Contributor

shollymanNov 1, 2023

Choose a reason for hiding this comment

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

Generally we try to avoid this kind of setter enforcement in the client library itself, as the library may not be aware of all valid values. Allow the methods to raise errors if there's issues (e.g at routine creation/update/etc).

Copy link

ContributorAuthor

LinchinNov 1, 2023

Choose a reason for hiding this comment

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

Thank you, this is really good to know! I will only check for type instead.

Copy link

ContributorAuthor

Linchin commentedNov 1, 2023

Is it worth adding the new field to an existing system/integration test related to routines, or are there extensive dependencies here to properly setup masking?

Data governance feels like a new CUJ that is not covered by the current system tests, with a unique set of requirements for the routine itself (e.g., can have only one argument, input and output must be of the same type, etc.). So I added a test case for it.