fix: connector builder cdk bump workflow #820
Conversation
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@dbgold17/update-cdk-bump-in-builder-workflow#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch dbgold17/update-cdk-bump-in-builder-workflowHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the connector builder CDK bump workflow by switching from manually updating CDK dependencies in multiple files to just updating the manifest-server Docker image tag. The change reflects the new deployment architecture where the airbyte/manifest-server image already includes the CDK version.
- Removed dependency on
publish_cdkandpublish_sdmjobs, replacing withpublish_manifest_server - Simplified the update process to only modify the manifest-server tag in Helm values file
- Eliminated Python setup, retry logic, and manual dependency compilation steps
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughUpdated Changes
Sequence Diagram(s)sequenceDiagram
participant Publish as publish_manifest_server
participant Updater as update-connector-builder
participant Repo as repo (values.yaml / build.gradle.kts / CDK_VERSION)
participant GH as GitHub (PR)
rect rgb(235,245,255)
Publish->>Updater: completes → triggers job
end
rect rgb(245,255,235)
Updater->>Repo: substitute VERSION into values.yaml\nupdate build.gradle.kts CDK reference\nwrite CDK_VERSION file
Updater->>GH: create PR with in-repo edits
GH-->>Updater: PR created
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
.github/workflows/publish.yml (1)
370-380: Consider escaping version strings in sed regex for robustness.The sed pattern
tag: ${PREVIOUS_VERSION}doesn't escape special regex characters. Semantic versions like1.0.0contain dots (.), which are regex metacharacters matching any character. While unlikely to cause issues in a well-formatted YAML file, this could theoretically match unintended patterns.Wdyt about using a safer sed pattern, either by escaping the version or using a different delimiter?
Here's a more robust approach:
# Escape special characters for sed PREV_ESCAPED=$(printf '%s\n' "${PREVIOUS_VERSION}" | sed -e 's/[\/&]/\\&/g') NEW_ESCAPED=$(printf '%s\n' "${VERSION}" | sed -e 's/[\/&]/\\&/g') sed -i "s|tag: ${PREV_ESCAPED}|tag: ${NEW_ESCAPED}|g" "oss/charts/v2/airbyte/values.yaml"Or using a different delimiter to reduce escaping needs:
sed -i "s|tag: ${PREVIOUS_VERSION}|tag: ${VERSION}|g" "oss/charts/v2/airbyte/values.yaml"(Note: Using
|as delimiter still requires escaping|in the version if it appears, but|is much less common in version strings than/.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/publish.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Publish Manifest Server to DockerHub
- GitHub Check: Publish SDM to DockerHub
- GitHub Check: Publish CDK version to PyPI
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
.github/workflows/publish.yml (2)
343-345: Job dependency refactoring aligns with simplified architecture. ✓Shifting from
publish_cdkandpublish_sdmtopublish_manifest_servermakes sense—since manifest-server now includes the CDK, this reduces complexity and removes unnecessary dependencies. Nice simplification!
346-354: Verify VERSION is set for manual workflow_dispatch triggers.Looking at the condition, when triggered via
workflow_dispatchwithupdate_connector_builder == 'true', there's no guarantee thatVERSIONis populated. If both the auto-detected and user-input versions are empty,VERSIONbecomes an empty string (see line 82), which would result in invalid YAML (tag:with no value).Can you confirm that the version is always expected to be set in practice, or should we add a validation check to fail fast if
VERSIONis empty?
dbgold17
had a problem deploying
to
Connector Builder
GitHub Actions
Failure
dbgold17
temporarily deployed
to
Connector Builder
GitHub Actions
Inactive
|
turns out using I'm opting to use sed instead even if less optimized for the task. The PR should always have a human reviewer anyway to catch mistakes. |
dbgold17
had a problem deploying
to
Connector Builder
GitHub Actions
Failure
dbgold17
temporarily deployed
to
Connector Builder
GitHub Actions
Inactive
Since @pedroslopez changed how we build and deploy connector builder using the
airbyte/manifest-serverimage, the process of updating the CDK version used by Builder in OSS and Cloud has become much simpler and this workflow that creates a PR automatically to update Builder to the latest CDK version needed to be altered.I tested this out by manually running this workflow (generating a pre-release CDK version).
Here's the PR that was created: https://github.com/airbytehq/airbyte-platform-internal/pull/18058/files
Summary by CodeRabbit