-
Notifications
You must be signed in to change notification settings - Fork 98
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
DENG-774 Change Control for active_users_aggregates #3687
Conversation
def is_github_identity(s): | ||
"""Check if the given string matches the format of a Github identity.""" | ||
return re.match(r"[mozilla]+\/[a-zA-Z0-9]+", s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Airflow task we need to filter out the Github teams otherwise DAGs cannot be generated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer we include the @
prefix that github uses for github identities. I assume we already have some normalization logic for labels that propagate to GCP if that's the reason we've omitted it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! I don't think I fully understand how all the pieces fit together with the bigger picture (e.g. I wouldn't have caught @scholtzan's comment about the Airflow task), but from what I'm able to follow it looks great. I just had a few minor refactoring suggestions.
Integration report for "Merge branch 'main' into DENG-774_map_labels_and_owners"
|
Integration report for "DENG-774 Update test property."
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some trouble reading through the validation section. I think it would be simpler to reason about if we required all github identities listed as owners
in metadata to be listed as CODEOWNERS for tables subject to this change control.
def is_github_identity(s): | ||
"""Check if the given string matches the format of a Github identity.""" | ||
return re.match(r"[mozilla]+\/[a-zA-Z0-9]+", s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer we include the @
prefix that github uses for github identities. I assume we already have some normalization logic for labels that propagate to GCP if that's the reason we've omitted it.
Integration report for "DENG-774 Syntax improvements and use of readlines() to read CODEOWNERS."
|
Integration report for "Merge branch 'main' into DENG-774_map_labels_and_owners"
|
…into DENG-774_map_labels_and_owners
Integration report for "Merge branch 'main' into DENG-774_map_labels_and_owners"
|
Integration report for "DENG-774 Update DAGs and notification."
|
Integration report for "DENG-774 Require at least on owner instead of exactly one."
|
Integration report for "DENG-774 Add tests scenarios and consider lines in CODEOWNERS starting with #."
|
Integration report for "DENG-774 Syntax update."
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc
Thank you for adding the tests. I think it's important to decide whether to require only owners
in CODEOWNERS, but I don't think that needs another round of review if a test is added.
After this is merged https://github.com/mozilla/bigquery-etl/settings/branch_protection_rules/4491948 should be updated to require the validate-metadata
check since it isn't required presently.
Implements DENG-774
For modifications to schemas in restricted namespaces (see
CODEOWNERS
):