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

connectors-ci: pass PR id to airbyte-ci #26504

Merged
merged 40 commits into from
May 30, 2023

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented May 24, 2023

What

1/2 to complete #24322
Closes #25279
We want to get the pull request id (aka pull request number) in the CI context to:

  • Add PR link to test reports that are stored to S3
  • Check if the PR is draft and stop the test pipeline if the PR is still in draft mode
  • Write test reports as comments PR

How

  • Pass the PR ID from GHA to airbyte-ci as an env var
  • Retrieve the PR object and store it in the context object
  • Make the test report write comment to PRs on context teardown

🚨 User Impact 🚨

  • The automated test on PR will be re-enabled, will help assess their stability or debug their instability at scalre
  • The PR comment will not be posted until we set the PRODUCTION env var to True.

@alafanechere alafanechere marked this pull request as ready for review May 25, 2023 08:23
@octavia-squidington-iii
Copy link
Collaborator

🔥 source-pokeapi test report 🔥

⏲️ Total pipeline duration: 115 seconds

Step Result
Connector version semver check.
Connector version increment check.
QA checks
Code format checks
Connector package install
Build source-pokeapi docker image for platform linux/x86_64
Unit tests
Integration tests 🟡
Acceptance tests
🔗 View the logs here

Please note that tests are only run on PR ready for review.
Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-pokeapi test

@octavia-squidington-iii octavia-squidington-iii removed the area/connectors Connector related issues label May 25, 2023
@alafanechere alafanechere requested review from bnchrch and evantahler and removed request for bnchrch May 25, 2023 12:53
@evantahler
Copy link
Contributor

Notes from Retro:

  • maybe replace the 🔥 emoji with the connector icons
  • make sure the comment is updated if you post on success too
  • probably also want the build summary

@octavia-squidington-iii
Copy link
Collaborator

source-pokeapi test report (commit d3f343fa30) - ❌

⏲️ Total pipeline duration: 83 seconds

Step Result
Connector version semver check.
Connector version increment check.
QA checks
Code format checks
Connector package install
Build source-pokeapi docker image for platform linux/x86_64
Unit tests
Acceptance tests

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-pokeapi test

@octavia-squidington-iii
Copy link
Collaborator

source-openweather test report (commit d3f343fa30) - ✅

⏲️ Total pipeline duration: 163 seconds

Step Result
Connector version semver check.
Connector version increment check.
QA checks
Code format checks
Connector package install
Build source-openweather docker image for platform linux/x86_64
Unit tests
Acceptance tests

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-openweather test

@alafanechere
Copy link
Contributor Author

@evantahler I did the following improvements:

  • Icons are displayed
  • An emoji in the comment title shows the global status
  • I added the commit for which the test ran in the commit title

I believe we should not update comments and create one comment per test run as it helps tracking which commit led to different test results. And it's tedious to update a comment because our jobs are stateless and I can keep the comment ID around to update it on a different pipeline run.

This reverts commit c2f23cc.
This reverts commit 483f6c5.
This reverts commit 5379b87.
This reverts commit d24ede4.
This reverts commit 5572910.
@octavia-squidington-iii octavia-squidington-iii removed area/connectors Connector related issues area/documentation Improvements or additions to documentation labels May 26, 2023
Comment on lines +11 to +12
#- "airbyte-integrations/connectors/**"
#- ".github/workflows/connector_integration_test_single_dagger.yml"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to current infra problems it's not safe to enable the automated test pipeline right now - concurrent jobs are blocking each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alafanechere if im reading this right then this workflow will run on "all" opened/ready for review PRs

Do we not want to keep these path filters so it runs on "all connector related" prs instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pipeline will eventually run on all PRs to make the required check pass...

Comment on lines +14 to +16
- opened
- synchronize
- ready_for_review
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pipeline will run when:

  • the pr is opened
  • when a commit is pushed to the pr
  • when the pr is made ready for review

But the tests will run only if the PR is not in draft mode thank to this change

@evantahler
Copy link
Contributor

evantahler commented May 26, 2023

  • Icons are displayed

NICE

  • An emoji in the comment title shows the global status

  • I added the commit for which the test ran in the commit title

I believe we should not update comments and create one comment per test run as it helps tracking which commit led to different test results. And it's tedious to update a comment because our jobs are stateless and I can keep the comment ID around to update it on a different pipeline run.

Ok, that's fine for now. We can revisit this if it gets annoying. As a note, you can see a comment's history if you do want it:

Screenshot 2023-05-26 at 8 47 27 AM

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.

One small comment. Other than that LGTM!

Comment on lines +11 to +12
#- "airbyte-integrations/connectors/**"
#- ".github/workflows/connector_integration_test_single_dagger.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

@alafanechere if im reading this right then this workflow will run on "all" opened/ready for review PRs

Do we not want to keep these path filters so it runs on "all connector related" prs instead?

@alafanechere
Copy link
Contributor Author

One small comment. Other than that LGTM!

Do we not want to keep these path filters so it runs on "all connector related" prs instead?

@bnchrch If we want to create a required PR check it'll have to run on all PRs...
This portion of the code will guarantee that the Global CI check will be updated to success even if not connector got tested on an unrelated PR:

click.secho("No connector were selected for testing.", fg="yellow")
update_global_commit_status_check_for_tests(ctx.obj, "success")
return True

@alafanechere alafanechere enabled auto-merge (squash) May 30, 2023 07:55
@alafanechere alafanechere merged commit 312a361 into master May 30, 2023
20 checks passed
@alafanechere alafanechere deleted the augustin/connectors-ci/pass-pr-id-to-pipelines branch May 30, 2023 08:15
marcosmarxm pushed a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run connectors tests only when PR is ready for review
4 participants