-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
airbyte-ci: failure status to GHA step #42077
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
@alafanechere this is different than the status check option that we talked about because the live/regression tests are only run manually right now, so there's not necessarily a PR with a status check that can be updated. This feels like the fastest/easiest way to surface the failure. |
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.
The reason the workflow does not exit with 1
when a test fail is because I find it useful to differentiate error in workflow / airbyte-ci
execution vs test failure.
We (as tooling team) should guarantee that this workflow is stable, if it's failing it means we have something to fix.
We have the global status check for connectors which has the role of showing global failure or success of CI for modified connectors on the PR
Fair enough @alafanechere. Updating to use status checks instead. (Not quite finished testing yet.) |
/approve-regression-tests
|
502cf5a
to
8ada4e8
Compare
/approve-regression-tests
|
/approve-regression-tests ref=9e48bc9226d17e6e6fd869fcc893b7b143425412
|
/approve-regression-tests ref=catherine/live-tests-status-to-gha
|
dda679f
to
6da2e07
Compare
/approve-regression-tests ref=catherine/live-tests-status-to-gha
|
/approve-regression-tests connector-name=source-faker ref=catherine/live-tests-status-to-gha
|
c3573a4
to
c042af8
Compare
/approve-regression-tests connector-name=source-faker ref=catherine/live-tests-status-to-gha
|
c042af8
to
9b5a7ef
Compare
/approve-regression-tests connector-name=source-faker ref=catherine/live-tests-status-to-gha
|
@@ -218,6 +218,13 @@ def dagger_cloud_url(self) -> Optional[str]: | |||
def remote_storage_enabled(self) -> bool: | |||
return self.is_ci and bool(self.ci_report_bucket) and bool(self.ci_gcp_credentials) | |||
|
|||
def _should_send_status_check(self) -> bool: | |||
should_send = self.is_pr or any( | |||
self.pipeline_name.startswith(override) for override in MANUAL_PIPELINE_STATUS_CHECK_OVERRIDE_PREFIXES |
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.
@alafanechere I don't love this way of doing things but found that if I didn't override this here, I had to mark the regression test run as a PR. That created duplicated status checks: one called Regression Tests
and one called Regression Tests on <source>
. Regression Tests on <source>
is what's created here, and has links to the report etc. so it's the only one I really want. If you have a suggestion of a nicer way to do this let me know.
I also found that Regression Tests on <source>
wasn't getting sent unless the global status check argument was present, which I found surprising. Is that intentional?
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.
That created duplicated status checks
This is intentional for usual connector testing: one global check (required) for all connectors being tested on the PR, and one for each connectors modified on the pr.
IMO you should make theRegression Tests
one the required check (to ensure that it has been run on all modified connectors).Regression Tests on <source>
could be just used to access reports. The tricky thing would be to markRegression Tests
pass even if it's not run on non certified connectors which could be modified in the same PR as a certified connector.
I also found that Regression Tests on
I don't think it's intentional, did you get the reason why?
1391175
to
3e38569
Compare
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'm not sure I get the necessity of 3 different status check.
Can we have a single required one:
- When no connector is modified on a PR:
connectors_tests.yml
marks it as successful with inline GitHub API calls. - When connectors are modified:
- If certified connectors in modified connectors:
airbyte-ci connectors test
sets it as pending (at the same level where the global status check is sent) - If no certified connectors are modified:
airbyte-ci connectors test
marks it as successful.
- If certified connectors in modified connectors:
- The slash command marks it as successful
Am I missing something or getting your intended flow wrong?
I don't think its required to make regression_tests.yml
change the status as results are not deterministics ATM.
@@ -60,9 +63,32 @@ jobs: | |||
[1]: ${{ steps.job-vars.outputs.run-url }} | |||
|
|||
- name: Approve regression tests | |||
id: approve | |||
id: approve-regression-tests |
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've the impression we have three different context values:
Regression Tests on ${{ github.event.inputs.connector-name }}
Regression Test Results Reviewed and Approved
Regression tests
(set in thesubcommand
ofregression_tests.yml
Can you share how these three articulate, I'm not sure I get why we need 3 different ones.
@@ -218,6 +218,13 @@ def dagger_cloud_url(self) -> Optional[str]: | |||
def remote_storage_enabled(self) -> bool: | |||
return self.is_ci and bool(self.ci_report_bucket) and bool(self.ci_gcp_credentials) | |||
|
|||
def _should_send_status_check(self) -> bool: | |||
should_send = self.is_pr or any( | |||
self.pipeline_name.startswith(override) for override in MANUAL_PIPELINE_STATUS_CHECK_OVERRIDE_PREFIXES |
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.
That created duplicated status checks
This is intentional for usual connector testing: one global check (required) for all connectors being tested on the PR, and one for each connectors modified on the pr.
IMO you should make theRegression Tests
one the required check (to ensure that it has been run on all modified connectors).Regression Tests on <source>
could be just used to access reports. The tricky thing would be to markRegression Tests
pass even if it's not run on non certified connectors which could be modified in the same PR as a certified connector.
I also found that Regression Tests on
I don't think it's intentional, did you get the reason why?
This reverts commit bc4cd3d0b8f1806b77d27c2feb88e4ef8ae51f78.
3e38569
to
ece04fd
Compare
When running tests, test failures aren't being surfaced in the GHA.
This PR changes that so we now get a failure status for the workflow if manually-run regression tests fail.
The status check for an individual regression test run can be updated using
/approve-regression-tests connector-name=<name>
. The global regression test status check can still be updated using/approve-regression-tests
with no connector name argument.Closes #41989.