require extra review for breaking change messages #31361

pedroslopez · 2023-10-12T21:50:36Z

What

We want to add Kat as a required reviewer when breaking change releases are made. This adds some logic to our existing checks to do so.

This also sets request-reviewers: true on our action so reviews are automatically requested when required.

Close #31141

How

Created new group, breaking-change-reviewers of which Kat is the only member currently to make a bit more flexible if we do want to change who gets notified or add more people.
Refactor mandatory reviewer logic to support multiple required review groups at a time, rather than just requesting the first group.
Add a method to get diff from connector metadata, + refactor the current method to get acceptance test config changes to use a common method.
- Since it's not just acceptance test config changes that cause required reviewers, also moved out some code from acceptance_test_config_checks to required-reviewer_checks

vercel · 2023-10-12T21:50:37Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2023 11:20pm

pedroslopez · 2023-10-12T21:50:47Z

Current dependencies on/for this PR:

master
- PR fix running connector-ops package tests via airbyte-ci #31478
  - PR require extra review for breaking change messages #31361 👈

This comment was auto-generated by Graphite.

pedroslopez · 2023-10-14T00:08:47Z

.github/workflows/connector_teams_review_requirements.yml

@@ -39,4 +39,5 @@ jobs:
        with:
          status: ${{ steps.get-mandatory-reviewers.outputs.MANDATORY_REVIEWERS  }}
          token: ${{ secrets.OCTAVIA_4_ROOT_ACCESS }}
+          request-reviews: true


This action now supports auto requesting reviews! Specially since the check isn't required, I think it's important to add else reviews can be missed

pedroslopez · 2023-10-14T00:08:58Z

airbyte-ci/connectors/connector_ops/connector_ops/acceptance_test_config_checks.py

@@ -13,6 +13,7 @@
 TEST_STRICTNESS_LEVEL_REVIEWERS = {"connector-operations"}
 GA_BYPASS_REASON_REVIEWERS = {"connector-operations"}
 GA_CONNECTOR_REVIEWERS = {"gl-python"}
+BREAKING_CHANGE_REVIEWERS = {"breaking-change-reviewers"}


new group - atm kat is the only member but can add as needed

pedroslopez · 2023-10-14T00:11:29Z

airbyte-ci/connectors/connector_ops/connector_ops/acceptance_test_config_checks.py

    if backward_compatibility_changes:
-        return [{"any-of": list(BACKWARD_COMPATIBILITY_REVIEWERS)}]
+        required_reviewers.append({"name": "Backwards Compatibility Test Skip", "teams": list(BACKWARD_COMPATIBILITY_REVIEWERS)})
    if test_strictness_level_changes:
-        return [{"any-of": list(TEST_STRICTNESS_LEVEL_REVIEWERS)}]
+        required_reviewers.append({"name": "Acceptance Test Strictness Level", "teams": list(TEST_STRICTNESS_LEVEL_REVIEWERS)})
    if ga_bypass_reason_changes:
-        return [{"any-of": list(GA_BYPASS_REASON_REVIEWERS)}]
+        required_reviewers.append({"name": "GA Acceptance Test Bypass", "teams": list(GA_BYPASS_REASON_REVIEWERS)})
    if important_connector_changes:
-        return list(GA_CONNECTOR_REVIEWERS)
-    return []
+        required_reviewers.append({"name": "GA Connectors", "teams": list(GA_CONNECTOR_REVIEWERS)})
+    if breaking_change_changes:
+        required_reviewers.append({"name": "Breaking Changes", "teams": list(BREAKING_CHANGE_REVIEWERS)})
+
+    return required_reviewers


This used to have early returns, which meant cases where multiple requirements are involved didn't work. Instead, we're adding multiple requirement groups that all have to pass. The contents of this dict is based on the format laid out in https://github.com/Automattic/action-required-review#requirements-format

As seen in the example, they list out multiple review groups that each have a name and team list.

pedroslopez · 2023-10-14T00:37:38Z

airbyte-ci/connectors/connector_ops/connector_ops/required_reviewer_checks.py

moved all required reviewer-related stuff here since it was no longer just for acceptance test config

alafanechere

Super cool!

alafanechere · 2023-10-16T18:33:07Z

airbyte-ci/connectors/connector_ops/pyproject.toml

@@ -30,7 +30,7 @@ freezegun = "^1.1.0"

 [tool.poetry.scripts]


please bump the package version 🙏

alafanechere · 2023-10-16T18:33:37Z

airbyte-ci/connectors/connector_ops/connector_ops/acceptance_test_config_checks.py

@@ -38,43 +31,6 @@ def find_connectors_with_bad_strictness_level() -> List[utils.Connector]:
    return connectors_with_bad_strictness_level


-def find_changed_important_connectors() -> Set[utils.Connector]:


Thanks for moving it to a separate module!

alafanechere · 2023-10-16T18:34:50Z

airbyte-ci/connectors/connector_ops/connector_ops/required_reviewer_checks.py

+    if backward_compatibility_changes:
+        required_reviewers.append({"name": "Backwards Compatibility Test Skip", "teams": list(BACKWARD_COMPATIBILITY_REVIEWERS)})
+    if test_strictness_level_changes:
+        required_reviewers.append({"name": "Acceptance Test Strictness Level", "teams": list(TEST_STRICTNESS_LEVEL_REVIEWERS)})
+    if ga_bypass_reason_changes:
+        required_reviewers.append({"name": "GA Acceptance Test Bypass", "teams": list(GA_BYPASS_REASON_REVIEWERS)})
+    if important_connector_changes:
+        required_reviewers.append({"name": "GA Connectors", "teams": list(GA_CONNECTOR_REVIEWERS)})
+    if breaking_change_changes:
+        required_reviewers.append({"name": "Breaking Changes", "teams": list(BREAKING_CHANGE_REVIEWERS)})


a mapping between change detection function and mandatory reviewers can be more readable

pedroslopez · 2023-10-16T23:20:24Z

Merge activity

Oct 16, 7:20 PM: Graphite rebased this pull request as part of a merge.
Oct 16, 7:32 PM: @pedroslopez merged this pull request with Graphite.

## What We want to add Kat as a required reviewer when breaking change releases are made. This adds some logic to our existing checks to do so. This also sets `request-reviewers: true` on our action so reviews are automatically requested when required. Close airbytehq#31141 ## How - Created new group, `breaking-change-reviewers` of which Kat is the only member currently to make a bit more flexible if we do want to change who gets notified or add more people. - Refactor mandatory reviewer logic to support multiple required review groups at a time, rather than just requesting the first group. - Add a method to get diff from connector metadata, + refactor the current method to get acceptance test config changes to use a common method. - Since it's not just acceptance test config changes that cause required reviewers, also moved out some code from `acceptance_test_config_checks` to `required-reviewer_checks`

pedroslopez force-pushed the pedro/required-breaking-change-reviewers branch 2 times, most recently from f0af9c2 to 6a1780d Compare October 13, 2023 23:40

pedroslopez commented Oct 14, 2023

View reviewed changes

pedroslopez force-pushed the pedro/required-breaking-change-reviewers branch from a4fe92a to 102c25c Compare October 14, 2023 00:55

pedroslopez marked this pull request as ready for review October 14, 2023 00:57

pedroslopez requested review from alafanechere and a team October 14, 2023 01:03

alafanechere approved these changes Oct 16, 2023

View reviewed changes

pedroslopez force-pushed the pedro/required-breaking-change-reviewers branch from 102c25c to 4f384c4 Compare October 16, 2023 20:03

pedroslopez changed the base branch from master to pedro/fix_airbyte_ci_tests October 16, 2023 22:42

pedroslopez force-pushed the pedro/required-breaking-change-reviewers branch from bd156dd to af68e13 Compare October 16, 2023 22:42

pedroslopez mentioned this pull request Oct 16, 2023

fix running connector-ops package tests via airbyte-ci #31478

Merged

pedroslopez force-pushed the pedro/fix_airbyte_ci_tests branch from 74b02f1 to 6e0c579 Compare October 16, 2023 22:44

pedroslopez force-pushed the pedro/required-breaking-change-reviewers branch from af68e13 to 22205a5 Compare October 16, 2023 22:44

Base automatically changed from pedro/fix_airbyte_ci_tests to master October 16, 2023 23:19

pedroslopez and others added 12 commits October 16, 2023 23:20

additive requried reviewers

f53237b

try this out

10aa3a7

Automated Commit - Formatting Changes

120279f

fix test

22cdffd

better logic

dabc536

fix tests

3478fec

add it back

60973c3

add test for br ch

96e5455

fix status

6e77eb1

auto request reviews

e5aff3f

Automated Commit - Formatting Changes

217ac9e

rename

000ef28

pedroslopez and others added 3 commits October 16, 2023 23:20

bump version

f15e60c

use a map

c07666d

Automated Commit - Formatting Changes

c95b2ae

pedroslopez force-pushed the pedro/required-breaking-change-reviewers branch from 22205a5 to c95b2ae Compare October 16, 2023 23:20

pedroslopez merged commit c207a7f into master Oct 16, 2023
14 checks passed

pedroslopez deleted the pedro/required-breaking-change-reviewers branch October 16, 2023 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

require extra review for breaking change messages #31361

require extra review for breaking change messages #31361

pedroslopez commented Oct 12, 2023 •

edited

Loading

vercel bot commented Oct 12, 2023 •

edited

Loading

pedroslopez commented Oct 12, 2023 •

edited

Loading

pedroslopez Oct 14, 2023

pedroslopez Oct 14, 2023

pedroslopez Oct 14, 2023

pedroslopez Oct 14, 2023

alafanechere left a comment

alafanechere Oct 16, 2023

alafanechere Oct 16, 2023

alafanechere Oct 16, 2023

pedroslopez commented Oct 16, 2023 •

edited

Loading

		@@ -38,43 +31,6 @@ def find_connectors_with_bad_strictness_level() -> List[utils.Connector]:
		return connectors_with_bad_strictness_level


		def find_changed_important_connectors() -> Set[utils.Connector]:

require extra review for breaking change messages #31361

require extra review for breaking change messages #31361

Conversation

pedroslopez commented Oct 12, 2023 • edited Loading

What

How

vercel bot commented Oct 12, 2023 • edited Loading

pedroslopez commented Oct 12, 2023 • edited Loading

pedroslopez Oct 14, 2023

Choose a reason for hiding this comment

pedroslopez Oct 14, 2023

Choose a reason for hiding this comment

pedroslopez Oct 14, 2023

Choose a reason for hiding this comment

pedroslopez Oct 14, 2023

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

alafanechere Oct 16, 2023

Choose a reason for hiding this comment

alafanechere Oct 16, 2023

Choose a reason for hiding this comment

alafanechere Oct 16, 2023

Choose a reason for hiding this comment

pedroslopez commented Oct 16, 2023 • edited Loading

Merge activity

pedroslopez commented Oct 12, 2023 •

edited

Loading

vercel bot commented Oct 12, 2023 •

edited

Loading

pedroslopez commented Oct 12, 2023 •

edited

Loading

pedroslopez commented Oct 16, 2023 •

edited

Loading