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: CLI exposes CI requirements #34218

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Jan 12, 2024

What

Relates to #34076

We currently have to define the dagger version to use at two places:

  • client side: the dagger-io python package version
  • engine/infra side: the dagger engine image version to run on the infra

On the infra side we have pre-populated runners using different dagger version: 0.6.4 and 0.9.5 .
The targeted runner for a CI job is currently in the runs-on field of our GHA workflows.
We can face CI failures when we have a mismatch between the client and infra side dagger version.

Mismatch can happen when:

  • A dagger version change only happened client side and the runners are not yet declared on the infra.
  • A workflow file on a branch is not up to date with master, still targeting an "old" runner, while the airbyte-ci binary in use is the latest.

Fixing these mismatch scenario will make the Dagger upgrade / airbyte-ci version pinning a lot easier.

How

  1. Make airbyte-ci define its CI requirements, (only the dagger version ATM): we expose a new --ci-requirements option to the commands in the CI. It outputs a JSON payload, currently {"dagger_version": "0.9.5"}
  2. airbyte-ci: compute GHA runs-on from --ci-requirements #34220 34220: In our GHA worfklows using airbyte-ci: Run an initial get-ci-runner job on a GHA runner which will call --ci-requirements for the specific command that will be run in a next job. The output of get-ci-runner is used as the runs-on field value of the main job.

We'll be able to introduce more complex scenario, like picking a runner size dynamically, by changing the output of --ci-requirements. We'll have to adapt workflows accordingly.

Copy link
Contributor Author

alafanechere commented Jan 12, 2024

In this stack: 🔧 Add CI requirements to Airbyte CLI(?)

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @alafanechere and the rest of your teammates on Graphite Graphite

@alafanechere alafanechere force-pushed the augustin/01-11-airbyte-ci_CLI_exposes_CI_requirements branch 2 times, most recently from fb831ed to 91b9c91 Compare January 12, 2024 15:02
Copy link

vercel bot commented Jan 12, 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 Jan 15, 2024 11:08am

@alafanechere alafanechere force-pushed the augustin/01-11-airbyte-ci_CLI_exposes_CI_requirements branch 7 times, most recently from 5163fd2 to 18552bb Compare January 12, 2024 16:03
@alafanechere alafanechere force-pushed the augustin/01-11-airbyte-ci_CLI_exposes_CI_requirements branch from 18552bb to 9087f08 Compare January 12, 2024 16:20
@alafanechere alafanechere marked this pull request as ready for review January 12, 2024 16:20
@alafanechere alafanechere requested a review from a team January 12, 2024 16:20
@@ -121,3 +126,27 @@ def decorated_function(*args: Any, **kwargs: Any) -> Any: # noqa: ANN401
return f(*args, **kwargs)

return decorated_function


def click_ci_requirements_option() -> Callable[[FC], FC]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can eventually add arguments to this decorator to customize requirements according to commands.



@dataclass
class CIRequirements:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used a dataclass here in case we want to make the CI requirements payload a bit more complex. It does not have custom attributes at the moment but we can easily add some if needed.

@alafanechere alafanechere force-pushed the augustin/01-11-airbyte-ci_CLI_exposes_CI_requirements branch from 9087f08 to 211409e Compare January 15, 2024 11:04
@alafanechere alafanechere force-pushed the augustin/01-11-airbyte-ci_CLI_exposes_CI_requirements branch from 211409e to 88c7b9e Compare January 15, 2024 11:08
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.

@@ -83,6 +88,9 @@ def check_local_docker_configuration() -> None:


def is_dagger_run_enabled_by_default() -> bool:
if CI_REQUIREMENTS_OPTION_NAME in sys.argv:
Copy link
Contributor

Choose a reason for hiding this comment

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

💅 Note for future: We do need to invert this at some point where commands specifiy if they need dagger run or not.

@alafanechere
Copy link
Contributor Author

/approve-and-merge reason="format is known to be broken on airbyte-ci changes, #34220 will solve it"

@octavia-approvington
Copy link
Contributor

After a careful ML study,
we think this looks okay.
imagine code being okay

@octavia-approvington octavia-approvington merged commit f3503ae into master Jan 15, 2024
22 of 23 checks passed
@octavia-approvington octavia-approvington deleted the augustin/01-11-airbyte-ci_CLI_exposes_CI_requirements branch January 15, 2024 18:09
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
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.

None yet

3 participants