-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: group shell commands for better layering #30279
Conversation
ce90c8d
to
78e029a
Compare
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.
This is a very neat change, thank you!
Approving
Could you please:
- bump the version in
pyproject.toml
- add a changelog entry in the README
- add a very small unit test in
airbyte-ci/connectors/pipelines/tests/test_utils.py
to testsh_dash_c
] | ||
) | ||
) | ||
.with_env_variable("VERSION", "24.0.2") |
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.
Can we use the constant that determine the docker version we use here (DOCKER_VERSION
)?
It will make more obvious that VERSION
means docker version :)
https://github.com/airbytehq/airbyte/blob/master/airbyte-ci/connectors/pipelines/pipelines/consts.py#L29
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.
Yes please
Thanks for the review! Addressed all comments. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This PR introduces a
sh_dash_c
utility function to pass to.with_exec
to avoid chaining these in cases where we actually want the commands to pass or fail atomically.The commands are grouped together by &&-ing them and the group is prefixed with a
set -o xtrace
which makes the logs easier to understand.Fixes #29735