Regression tests: require manual approval for certified connectors #41627
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
clnoll
force-pushed
the
catherine/require-regression-tests
branch
2 times, most recently
from
a3f9ccd
to
49b1f29
Compare
There was a problem hiding this comment.
Approving as the logic LGTM.
Please consider cleaning up the approve-regression-tests-command.yml, I spotted a couple of extra steps / env vars that are not required for what you want to do.
In any case feel free to merge, test the workflow once on master and tweak it on a new PR, it's very hard to merge an error free new workflow as we can't test them until they're on master.
| @@ -124,6 +125,17 @@ async def test( | |||
|
|
|||
| if ctx.obj["selected_connectors_with_modified_files"]: | |||
| update_global_commit_status_check_for_tests(ctx.obj, "pending") | |||
| if any([connector.support_level == "certified" for connector in ctx.obj["selected_connectors"]]): | |||
There was a problem hiding this comment.
It might be worth adding a comment about why we're emitting this status check even if the pipeline is not running regression tests. (It could be added the hacks.py)
| @@ -27,6 +27,7 @@ jobs: | |||
| test-performance | |||
| publish-java-cdk | |||
| connector-performance | |||
| approve-regression-tests | |||
There was a problem hiding this comment.
I always find difficult to understand slash commands as these commands must match <workflow-name>-command ...
There was a problem hiding this comment.
Yes that wasn't obvious to me either.
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.inputs.pr }} | ||
| # Cancel any previous runs on the same branch if they are still in progress | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
💅 This block could be superfluous right? We don't really care about concurrency if I'm not mistaken.
There was a problem hiding this comment.
Yes that's probably true. Removed.
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
This env var is not used if I'm not mistaken
There was a problem hiding this comment.
This is required to us the gh cli.
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
This env var is not used
Co-authored-by: Augustin <augustin@airbyte.io>
Co-authored-by: Augustin <augustin@airbyte.io>
Co-authored-by: Augustin <augustin@airbyte.io>
Co-authored-by: Augustin <augustin@airbyte.io>
clnoll
force-pushed
the
catherine/require-regression-tests
branch
from
7b39f97
to
1df7172
Compare
clnoll
force-pushed
the
catherine/require-regression-tests
branch
from
1df7172
to
754e581
Compare
…41627) Co-authored-by: Augustin <augustin@airbyte.io>
What
Modifies the CI pipeline for certified connectors so that a manual regression test approval is required.
How
airbyte-cito add a status check for manual regression test approval if any of the connectors changed in the PR are certified. This status check will initially be in a failing state.approve-regression-tests, that will update the status check to succeed.Developer impact
When a PR is opened that has changes to one or more certified connector, the developer will see a failing status check in the list of CI checks.
Once regression tests have been run and the reviewer has approved them, they can add an
/approve-regression-testsslash-command as a comment in the PR, and the status check will succeed.