ci-connector-ops: declare review requirements on source connectors changes #20303

alafanechere · 2022-12-09T16:00:03Z

What

Closes #20304

This GitHub action can help us check if a set of teams gave a review on a PR by creating a "review requirements" file.
I made a new command in ci-connector-ops to write this file according to the current changes in the branch.

I changed the connector_ops_ci.yml workflow to:

Call the new write-review-requirements-file command to write the requirements file. This file structure is defined by the Automattic/action-required-review@v3 action
Run the Automattic/action-required-review@v3 if this file was written.

When the requirements are not met, a status check as this one will appear:

The new following review requirements rules:

If a PR touches a GA connector: a review from gl-python team is required
If a PR changes the test strictness level in acceptance-test-config.yml: a review from connector-operations teams is required
If a PR changes backward compatibility test in acceptance-test-config.yml: a review from connector-operations or connector-extensibility is required.

Suggested review order:

`ci_connector_ops` python package

The new command to write the review requirements file:

airbyte/tools/ci_connector_ops/ci_connector_ops/sat_config_checks.py

Line 76 in 9da4887

def write_review_requirements_file():
Implement get_changed_acceptance_test_config to track changes on acceptance-test-config.yml:

airbyte/tools/ci_connector_ops/ci_connector_ops/utils.py

Line 55 in 9da4887

def get_changed_acceptance_test_config(diff_regex: Optional[str]=None) -> Set[str]:
"Integration" tests:

airbyte/tools/ci_connector_ops/tests/test_sat_config_checks.py

Line 90 in 9da4887

def test_find_mandatory_reviewers_backward_compatibility(capsys, not_ga_backward_compatibility_change_expected_team):

`connector_ops_ci.yml` GitHub action workflow

Add a new check-review-requirements job

airbyte/.github/workflows/connector_ops_ci.yml

Line 25 in 45d990a

check-review-requirements:

tools/ci_connector_ops/setup.py

erohmensing

This looks like it will be super helpful! Might be worth sharing this action and how you implemented it (Loom?) to other teams who may be unsatisfied with how static codeowners is

erohmensing · 2022-12-12T14:26:34Z

tools/ci_connector_ops/ci_connector_ops/sat_config_checks.py

+        requirements_file_content = [{
+            "name": "Required reviewers from the connector org teams",
+            "paths": "unmatched",
+            "teams": mandatory_reviewers
+        }]


Cool idea to generate this on each run!

erohmensing · 2022-12-12T14:31:29Z

tools/ci_connector_ops/ci_connector_ops/sat_config_checks.py

+        if isinstance(mandatory_reviewer, dict):
+            teams += mandatory_reviewer["any-of"]
+        else:
+            teams.append(mandatory_reviewer)


Doing the any-of check here feels a little un-DRY, since you also manually add any-of in find_mandatory_reviewers -- maybe you could put this logic of adding the any-of into a helper method you call in find_mandatory_reviewers?

tools/ci_connector_ops/ci_connector_ops/sat_config_checks.py

erohmensing · 2022-12-12T14:39:29Z

.github/workflows/connector_ops_ci.yml

+        if: steps.write-review-requirements-file.outputs.REVIEW_REQUIREMENTS_FILE != ''
+        uses: Automattic/action-required-review@v3
+        with:
+          status: ${{ steps.get-mandatory-reviewers.outputs.MANDATORY_REVIEWERS  }}


To be clear, generating and outputting this separately is a requirement of the github action, right? We can't generate that into connector_org_review_requirements.yaml?

Exactly, I added the MANDATORY_REVIEWERS to conveniently display a detailed status like A review is required from teams: A, B, C instead of the default Review required status. But all the essential information is captured in the REVIEW_REQUIREMENTS_FILE whose format is defined by the external action.

Cool, I figured. Strange that you can't configure different status messages for the different sets of requirements in the action 😄

Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com>

YowanR · 2022-12-12T15:14:26Z

This makes sense at a high-level to me. Adding @lazebnyi as a reviewer to make sure that GL is plugged into this conversation.

evantahler

I left a comment and that triggered a failing check!

https://github.com/airbytehq/airbyte/actions/runs/3680412195

[Invalid workflow file: .github/workflows/connector_ops_ci.yml#L49](https://github.com/airbytehq/airbyte/actions/runs/3680412195/workflow)
The workflow is not valid. .github/workflows/connector_ops_ci.yml (Line: 49, Col: 13): Unexpected symbol: '""'. Located at position 74 within expression: steps.write-review-requirements-file.outputs.REVIEW_REQUIREMENTS_FILE != ""

alafanechere · 2022-12-13T09:35:24Z

@evantahler, there was a glitch in the boolean check to conditionally run the final action. I fixed it in e7202ff

evantahler

👍 assuming I don't get another failing comment :D

alafanechere added 2 commits December 9, 2022 16:43

test

df31bef

add new connector ops commands

2fc9d7f

octavia-squidington-iv added area/connectors Connector related issues connectors/source/amplitude labels Dec 9, 2022

bump version

ea61e5a

alafanechere requested a review from pedroslopez December 9, 2022 16:02

alafanechere marked this pull request as ready for review December 9, 2022 16:02

alafanechere requested a review from a team December 9, 2022 16:02

alafanechere temporarily deployed to more-secrets December 9, 2022 16:03 — with GitHub Actions Inactive

erohmensing reviewed Dec 9, 2022

View reviewed changes

tools/ci_connector_ops/setup.py Outdated Show resolved Hide resolved

alafanechere changed the title ~~ci-connector-ops: check if backward compatibility tests are disabled~~ ci-connector-ops: check if a review is required from the connector team Dec 9, 2022

alafanechere changed the title ~~ci-connector-ops: check if a review is required from the connector team~~ ci-connector-ops: check if backward compatibility tests are disabled Dec 9, 2022

alafanechere marked this pull request as draft December 9, 2022 16:38

test a new job

54d61e9

alafanechere temporarily deployed to more-secrets December 9, 2022 16:51 — with GitHub Actions Inactive

test a new job

8b60b2e

alafanechere temporarily deployed to more-secrets December 9, 2022 16:58 — with GitHub Actions Inactive

alafanechere temporarily deployed to more-secrets December 9, 2022 16:59 — with GitHub Actions Inactive

test action

239c678

alafanechere temporarily deployed to more-secrets December 9, 2022 17:10 — with GitHub Actions Inactive

test action

d513159

alafanechere temporarily deployed to more-secrets December 9, 2022 17:18 — with GitHub Actions Inactive

test action

0d1034a

alafanechere temporarily deployed to more-secrets December 9, 2022 17:25 — with GitHub Actions Inactive

test action

0c44bd4

erohmensing approved these changes Dec 12, 2022

View reviewed changes

Update tools/ci_connector_ops/ci_connector_ops/sat_config_checks.py

b11a4fe

Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com>

alafanechere temporarily deployed to more-secrets December 12, 2022 14:55 — with GitHub Actions Inactive

alafanechere requested a review from lazebnyi December 12, 2022 15:07

alafanechere added 2 commits December 12, 2022 21:49

fix absence check

e49b2a6

enable workflow_dispatch

3e1aaf4

alafanechere temporarily deployed to more-secrets December 12, 2022 20:53 — with GitHub Actions Inactive

lazebnyi approved these changes Dec 12, 2022

View reviewed changes

evantahler approved these changes Dec 12, 2022

View reviewed changes

evantahler requested changes Dec 12, 2022

View reviewed changes

fix boolean check

e7202ff

alafanechere requested a review from evantahler December 13, 2022 09:35

alafanechere temporarily deployed to more-secrets December 13, 2022 09:36 — with GitHub Actions Inactive

fix boolean check

b3b35fe

alafanechere temporarily deployed to more-secrets December 13, 2022 09:46 — with GitHub Actions Inactive

alafanechere temporarily deployed to more-secrets December 13, 2022 09:47 — with GitHub Actions Inactive

update tests

b19e1d5

alafanechere temporarily deployed to more-secrets December 13, 2022 11:13 — with GitHub Actions Inactive

alafanechere temporarily deployed to more-secrets December 13, 2022 11:14 — with GitHub Actions Inactive

evantahler approved these changes Dec 13, 2022

View reviewed changes

alafanechere merged commit c2895c1 into master Dec 13, 2022

alafanechere deleted the augustin/ci-connector-ops/require-team-review branch December 13, 2022 22:55

github-actions bot mentioned this pull request Dec 15, 2022

Bump helm chart version reference to 0.43.5 #20526

Closed

octavia-squidington-iii mentioned this pull request Dec 18, 2022

Bump Airbyte version from 0.40.25 to 0.40.26 #20630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci-connector-ops: declare review requirements on source connectors changes #20303

ci-connector-ops: declare review requirements on source connectors changes #20303

alafanechere commented Dec 9, 2022 •

edited

erohmensing left a comment

erohmensing Dec 12, 2022

erohmensing Dec 12, 2022

erohmensing Dec 12, 2022

alafanechere Dec 12, 2022

erohmensing Dec 12, 2022

YowanR commented Dec 12, 2022

evantahler left a comment

alafanechere commented Dec 13, 2022

evantahler left a comment

ci-connector-ops: declare review requirements on source connectors changes #20303

ci-connector-ops: declare review requirements on source connectors changes #20303

Conversation

alafanechere commented Dec 9, 2022 • edited

What

The new following review requirements rules:

Suggested review order:

ci_connector_ops python package

connector_ops_ci.yml GitHub action workflow

erohmensing left a comment

Choose a reason for hiding this comment

erohmensing Dec 12, 2022

Choose a reason for hiding this comment

erohmensing Dec 12, 2022

Choose a reason for hiding this comment

erohmensing Dec 12, 2022

Choose a reason for hiding this comment

alafanechere Dec 12, 2022

Choose a reason for hiding this comment

erohmensing Dec 12, 2022

Choose a reason for hiding this comment

YowanR commented Dec 12, 2022

evantahler left a comment

Choose a reason for hiding this comment

alafanechere commented Dec 13, 2022

evantahler left a comment

Choose a reason for hiding this comment

alafanechere commented Dec 9, 2022 •

edited

`ci_connector_ops` python package

`connector_ops_ci.yml` GitHub action workflow