fix: combined publish flow for CDK/SDM #77
Conversation
Co-authored-by: Aaron ("AJ") Steers <[email protected]>
…om/airbytehq/airbyte-python-cdk into christo/finalize-cdk-sdm-publish-flow
…b.com/airbytehq/airbyte-python-cdk into christo/finalize-cdk-sdm-publish-flow
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: Packages-${{ github.run_id }} | ||
| path: dist |
There was a problem hiding this comment.
Adding this step is necessary to ensure that the SDM image has access to the build artifact and can utilize the correct CDK version
| @@ -13,6 +14,6 @@ RUN poetry config virtualenvs.create false \ | |||
| COPY airbyte_cdk ./airbyte_cdk | |||
|
|
|||
| # Build and install the package | |||
| RUN poetry build && pip install dist/*.whl | |||
| RUN pip install dist/*.whl | |||
There was a problem hiding this comment.
Updated this as it would overwrite the CDK version obtained from the build artifact
| @@ -4,6 +4,7 @@ WORKDIR /airbyte/integration_code | |||
|
|
|||
| # Copy project files needed for build | |||
| COPY pyproject.toml poetry.lock README.md ./ | |||
| COPY dist/*.whl ./dist/ | |||
There was a problem hiding this comment.
Copy the packages obtained from the build
There was a problem hiding this comment.
A comment + link to code on what builds it and why we think it'll be there would help!
📝 WalkthroughWalkthroughThe pull request includes the deletion of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Wdyt about having a discussion with the reviewers to clarify any points before merging? Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
Dockerfile (1)
7-7: Consider adding error handling for missing wheel files?The COPY command will fail silently if no wheel files exist in the dist directory. Maybe we could add a quick check before the copy? wdyt?
+RUN [ -d dist ] && [ "$(ls -A dist/*.whl 2>/dev/null)" ] || (echo "No wheel files found in dist directory" && exit 1) COPY dist/*.whl ./dist/.github/workflows/pypi_publish.yml (4)
3-9: Consider limiting the push trigger scopeThe current push trigger runs on all pushes to all branches. Would it make more sense to limit this to specific paths or branches to avoid unnecessary builds? For example:
on: push: + paths: + - 'airbyte/**' + - 'setup.py' + - 'pyproject.toml' + branches: + - main + - 'v*' workflow_dispatch:wdyt? 🤔
Line range hint
46-54: Add error handling for release uploadThe release upload step could benefit from some error handling. Maybe something like:
- name: Upload wheel to release if: startsWith(github.ref, 'refs/tags/v') uses: svenstaro/upload-release-action@v2 with: repo_token: ${{ secrets.GITHUB_TOKEN }} file: dist/*.whl tag: ${{ github.ref }} overwrite: true file_glob: true + fail_on_unmatched_files: trueThis would fail explicitly if no wheel files are found. What do you think? 🛠️
101-114: Enhance tag existence check robustnessThe tag check is good, but we could make it more resilient:
- name: Check for existing tag run: | + set -euo pipefail tag="airbyte/source-declarative-manifest:${{ env.VERSION }}" - if [ -z "$tag" ]; then + if [ -z "${tag:-}" ]; then echo "Error: VERSION is not set. Ensure the tag follows the format 'refs/tags/vX.Y.Z'." exit 1 fiThis adds better error handling and safer variable expansion. Thoughts? 🤔
115-124: Consider extracting platform list to reusable variableFor better maintainability, we could define platforms as a variable:
+ env: + PLATFORMS: linux/amd64,linux/arm64 - name: Build and push uses: docker/build-push-action@v5 with: context: . - platforms: linux/amd64,linux/arm64 + platforms: ${{ env.PLATFORMS }} push: trueThis makes it easier to update supported platforms in the future. What do you think? 💭
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/cdk-publish.yml(0 hunks).github/workflows/pypi_publish.yml(2 hunks)Dockerfile(2 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/cdk-publish.yml
🧰 Additional context used
🪛 actionlint
.github/workflows/pypi_publish.yml
72-72: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting
(shellcheck)
78-78: shellcheck reported issue in this script: SC2086:info:2:42: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
Dockerfile (1)
17-17: LGTM! Simple and effective approach.
Using pre-built wheels is more deterministic than building during the Docker build. Would you consider adding a quick version check after installation to ensure we got the right version? Something like:
RUN pip install dist/*.whl
+RUN pip freeze | grep airbyte-cdk.github/workflows/pypi_publish.yml (1)
22-28: Consider adding artifact retention policy
The artifact upload looks good, but we might want to add retention settings to manage storage. How about:
- uses: actions/upload-artifact@v4
with:
name: Packages-${{ github.run_id }}
+ retention-days: 5
path: |
/tmp/baipp/dist/*.whl
/tmp/baipp/dist/*.tar.gzAlso, could you verify that /tmp/baipp/dist/ is the correct path for the build artifacts? 🔍
| with: | ||
| name: Packages-${{ github.run_id }} | ||
| path: | | ||
| /tmp/baipp/dist/*.whl |
There was a problem hiding this comment.
Had to specify this path for uploading build artifacts as the /dist directory was not matching the path in the previous step
| steps: | ||
| - uses: actions/download-artifact@v4 | ||
| with: | ||
| name: Packages | ||
| name: Packages-${{ github.run_id }} |
There was a problem hiding this comment.
Had to make this name unique for each run to avoid 409 errors being thrown
| tags: | | ||
| airbyte/source-declarative-manifest:latest | ||
| airbyte/source-declarative-manifest:${{ env.VERSION }} | ||
| airbyte/source-declarative-manifest:${{ github.sha }} |
There was a problem hiding this comment.
@aaronsteers and @ChristoGrab: do we really want to push builds for sha tags to public dockerhub repo? Wouldn't it be too noisy?
What
Warning
99% sure the workflow_dispatch trigger is still broken. I ran into issues with the input version not being respected by the build step, leading to all sorts of shenanigans. Decided to prioritize getting the release trigger working to unblock us on publishing new CDK/SDM versions. Fow now, the following should run and work:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes