Skip to content
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: disable test reports PR comments #31871

Merged

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Oct 26, 2023

What

Closes #29300
We don't want PR comments anymore on connector CI test run.

How

  • Add a 'feature flag' disabling PR comments
  • Change the status details link to the HTML report when its ready

The global Connector CI status details will link to the GHA workflow.
The per connector status details will link to the GHA workflow while they're running and to the HTML report when they're done.

Why a 'feature flag' instead of deleting the commenting logic

When we will consider running the CI on community PRs I think having these comments back can be a good way to expose high-level results to the community contributors. HTML reports are currently private to not overly expose logs.

@vercel
Copy link

vercel bot commented Oct 26, 2023

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 26, 2023 7:29pm

Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

Copy link
Contributor

@bnchrch bnchrch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment around deadcode

But this is at a spot where im fine to pre-approve!

@@ -25,6 +25,8 @@
class ConnectorReport(Report):
"""A dataclass to build connector test reports to share pipelines executions results with the user."""

ENABLE_PR_COMMENTS: bool = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im a bit apprehensive of leaving unused code in the code base.

Code is cattle not a pet (ref)

I would argue that we remove it, unless theres a reason to keep it around that I may be missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we will consider running the CI on community PRs I think having these comments back can be a good way to expose high-level results to the community contributors. HTML reports are currently private to not overly expose logs.

@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Oct 26, 2023
@octavia-squidington-iv octavia-squidington-iv requested a review from a team October 26, 2023 17:15
@alafanechere alafanechere force-pushed the augustin/10-26-airbyte-ci_disable_test_reports_PR_comments branch from 8547b87 to 775117a Compare October 26, 2023 18:44
@octavia-squidington-iii octavia-squidington-iii removed the area/connectors Connector related issues label Oct 26, 2023
@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Oct 26, 2023
This reverts commit dee797b.
@octavia-squidington-iii octavia-squidington-iii removed the area/connectors Connector related issues label Oct 26, 2023
@alafanechere alafanechere enabled auto-merge (squash) October 26, 2023 19:29
@alafanechere alafanechere merged commit e477ddc into master Oct 26, 2023
21 checks passed
@alafanechere alafanechere deleted the augustin/10-26-airbyte-ci_disable_test_reports_PR_comments branch October 26, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Connector CI to report status to Github Status Checks not comments
3 participants