CI: Follow-up PR for SDM workflow #85
Conversation
aaronsteers
changed the title
📝 Walkthrough📝 WalkthroughWalkthroughThe modifications to the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/publish_sdm_connector.yml (2)
Line range hint
134-146: Simplify the tag existence checkThe tag check has a few potential improvements:
- The empty tag error seems misplaced since we already check
if: env.VERSION != ''- We could extract the tag into a variable at the job level for reuse
Would you consider this refactor?
- name: "Check for existing tag (version: ${{ env.VERSION || 'none' }} )" if: env.VERSION != '' run: | - tag="airbyte/source-declarative-manifest:${{ env.VERSION }}" - if [ -z "$tag" ]; then - echo "Error: VERSION is not set. Ensure the tag follows the format 'refs/tags/vX.Y.Z'." - exit 1 - fi + tag="airbyte/source-declarative-manifest:${{ env.VERSION }}" echo "Checking if tag '$tag' exists on DockerHub..." if DOCKER_CLI_EXPERIMENTAL=enabled docker manifest inspect "$tag" > /dev/null 2>&1; then echo "The tag '$tag' already exists on DockerHub. Skipping publish." exit 1 fi echo "No existing tag '$tag' found. Proceeding with publish." + echo "DOCKER_TAG=${tag}" >> $GITHUB_ENV
Line range hint
148-190: Consider using a matrix strategy for build configurationsI notice we have three similar build-push steps with identical platform configurations. What if we used a matrix strategy to reduce duplication? Here's a thought:
- name: Set up build matrix id: matrix run: | tags=() if [ -z "$VERSION" ]; then tags+=("${{ github.sha }}") else tags+=("$VERSION") if [ "$IS_PRERELEASE" = "false" ]; then tags+=("latest") fi fi echo "tags=${tags[@]}" >> $GITHUB_OUTPUT - name: Build and push if: github.event.inputs.dry_run == 'false' uses: docker/build-push-action@v5 with: context: . platforms: linux/amd64,linux/arm64 push: true tags: airbyte/source-declarative-manifest:${{ matrix.tag }} strategy: matrix: tag: ${{ fromJson(steps.matrix.outputs.tags) }}This would make the workflow more maintainable and easier to extend with new architectures or tags. What are your thoughts on this approach? 🤔
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/publish_sdm_connector.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/publish_sdm_connector.yml (1)
81-81: Consider documenting the default version behavior
Hey! I notice we're using '0.0.0dev0' as a default version. While this is a good safeguard, what do you think about:
- Adding a comment explaining when this default is used?
- Ensuring it follows semver format (maybe '0.0.0-dev.0')?
wdyt? 🤔
Let's check if this version format is consistent with other parts of the codebase:
Also applies to: 85-85
aaronsteers
temporarily deployed
to
DockerHub
GitHub Actions
Inactive
aaronsteers
temporarily deployed
to
DockerHub
GitHub Actions
Inactive
aaronsteers
changed the title
aaronsteers
temporarily deployed
to
DockerHub
GitHub Actions
Inactive
Creating this PR to fix any issues and follow-up from:
Manual tests confirmed:
Summary by CodeRabbit
New Features
Bug Fixes
Chores