-
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
connectors-ci: wrap airbyte-ci in dagger run #28869
connectors-ci: wrap airbyte-ci in dagger run #28869
Conversation
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.
Nice
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.
One question!
@@ -29,4 +29,5 @@ pytest = "^6.2.5" | |||
pytest-mock = "^3.10.0" | |||
|
|||
[tool.poetry.scripts] | |||
airbyte-ci = "pipelines.commands.airbyte_ci:airbyte_ci" | |||
airbyte-ci-bare = "pipelines.commands.airbyte_ci:airbyte_ci" |
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.
Love that were brining in dagger run
.
One question though why use airbyte-ci-bare
and no airbyte-ci --disable-dagger-run ...
?
Seems like that is possible and better ergonomics for users.
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.
But we need an executable that dagger run
calls right?
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.
rewording: We can indeed have airbyte-ci --disable-dagger-run
easily. But I'm not sure we can easily get rid of a second executable. The airbyte-ci-bare
use won't be documented...
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.
@bnchrch I'll try some things out to check how we could not rely on a second executable.
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.
It wasn't too strong of a suggestion.
Also I think I could've worded it better as discouraging the direct use of airbyte-ci-bare
by
- updating our CI tools to use
airbyte-ci --disable-dagger-run
as the prefered pattern - updating the
airbyte-ci
code to check for the--disable-dagger-run
flag
For example
import argparse
import sys
def run_command(command_list: List[str]) -> None:
try:
subprocess.run(command_list + sys.argv[1:], check=True)
except subprocess.CalledProcessError as e:
sys.exit(e.returncode)
parser = argparse.ArgumentParser()
parser.add_argument('--disable-dagger-run', type=bool, default=False)
def main():
args = parser.parse_args()
command_to_run = ["airbyte-ci-internal"]
if not args.disable_dagger_run:
command_to_run = ["dagger", "run"] + command_to_run
check_dagger_cli_install()
run_command(command_to_run)
```
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 renamed airbyte-ci-bare
to airbyte-ci-internal
.
You can use airbyte-ci --no-tui
to disable the terminal UI (and directly use airbyte-ci-internal
).
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.
@bnchrch I'll keep running airbyte-ci-internal
in the GHA action to prevent any regression
What
Closes #28690
The Dagger CLI offers a nice Terminal UI for Dagger pipeline execution.
To use this Terminal UI user should wrap pipeline execution with
dagger run airbyte-ci
.We don't want our user to care about the Dagger CLI installation and to remind wrapping the CLI with
dagger run
.How
airbyte-ci-bare
command: this is our original, unwrappedairbyte-ci
commandairbyte-ci
command as the call of the main function of thedagger_run.py
module. Itdagger run airbyte-ci-bare
+ arguments in a subprocessairbyte-ci-bare
to not alter the CI behavior we've had so far.🚨 User Impact 🚨
Users should benefit from this by updating their
airbyte-ci
installation with :pipx install airbyte-ci/connectors/pipelines
REVIEWERS PLEASE TRY THIS AT HOME