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

Fix live test image when running in CI #38772

Merged
merged 7 commits into from
May 30, 2024

Conversation

clnoll
Copy link
Contributor

@clnoll clnoll commented May 29, 2024

What

Fixes an issue where regression tests were using the same target & control containers when running in GHA.

This was flagged by @aldogonzalez8 when he realized that the target & control containers were hitting the same URLs, when they were expected to be different.

How

Avoid looking for a connection_id.txt file when in CI and getting the control container. When this file is present, we were building the control image off of it instead of pulling the latest image from Dockerhub, however it should only be used for the target image.

Closes https://github.com/airbytehq/airbyte-internal-issues/issues/7950.

@clnoll clnoll requested a review from a team as a code owner May 29, 2024 20:59
Copy link

vercel bot commented May 29, 2024

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 May 30, 2024 5:14pm

@clnoll clnoll changed the title Catherine/fix live tests image in ci Fix live test image when running in CI May 29, 2024
@clnoll clnoll requested review from bnchrch and alafanechere May 29, 2024 20:59
@clnoll clnoll force-pushed the catherine/fix-live-tests-image-in-ci branch 2 times, most recently from 41240a8 to 53b98ba Compare May 29, 2024 21:43
if container_id_path.exists():
return await get_container_from_id(dagger_client, container_id_path.read_text())
if is_ci and is_target:
container_id_path = Path("/tmp/container_id.txt")
Copy link
Contributor

@alafanechere alafanechere May 30, 2024

Choose a reason for hiding this comment

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

In order to not tie this utils to the regression test suite I'd suggest to make the airbyte-ci RegressionTests step write /tmp/<docker-image-name-with-tag>_container_id.txt and make this function check if such a file is available. It'd allow us to keep the original signature and avoid the problem we currently have.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if it's too much, happy to 👍 to unblock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alafanechere I made this change.

@clnoll clnoll requested a review from alafanechere May 30, 2024 12:50
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

@clnoll nice! Pre-approving! Can you double check it works? I wonder if it's risky to create file name with special characters like : (the image name with tag will contain a :).

@clnoll
Copy link
Contributor Author

clnoll commented May 30, 2024

@alafanechere the ":" indeed did not work so I'm just using the target image tag instead. Sound okay to you?

@alafanechere
Copy link
Contributor

@alafanechere the ":" indeed did not work so I'm just using the target image tag instead. Sound okay to you?

I think you can use the slugify helper.

@clnoll clnoll force-pushed the catherine/fix-live-tests-image-in-ci branch 2 times, most recently from a8122fd to f785f8e Compare May 30, 2024 17:03
@clnoll clnoll force-pushed the catherine/fix-live-tests-image-in-ci branch from f785f8e to 3170d30 Compare May 30, 2024 17:14
@clnoll clnoll merged commit 1eca3fc into master May 30, 2024
30 checks passed
@clnoll clnoll deleted the catherine/fix-live-tests-image-in-ci branch May 30, 2024 17:57
Copy link

sentry-io bot commented Jun 5, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ValueError: Only one value for selected-streams is allowed. Got 3 pipelines.helpers.execution.run_steps in get_it... View Issue
  • ‼️ ModuleNotFoundError: No module named 'pipeline' pipelines.airbyte_ci.connectors.test.steps.comm... View Issue

Did you find this useful? React with a 👍 or 👎

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.

3 participants