fix: account for dynamic CDK version #72
Conversation
| airbyte/source-declarative-manifest:latest | ||
| airbyte/source-declarative-manifest:${{ env.CDK_VERSION }} | ||
| airbyte/source-declarative-manifest:${{ env.CDK_VERSION }}-${{ github.run_number }} | ||
| airbyte/source-declarative-manifest:test |
There was a problem hiding this comment.
TODO: Revert the tags before merging to main
| paths: | ||
| - 'airbyte_cdk/pyproject.toml' # To only publish on CDK version change | ||
| - 'airbyte_cdk/**' # Check for any changes in the CDK directory | ||
| - '!airbyte_cdk/test/**' # Ignore changes in test CDK test directory |
There was a problem hiding this comment.
Since the version in pyproject is static and doesn't reflect the actual dynamic CDK version, we default to pushing a new image on any changes to the CDK (except test files)
📝 WalkthroughWalkthroughThe pull request modifies the GitHub Actions workflow file Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Wdyt about these suggestions? 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 (3)
.github/workflows/cdk-publish.yml (3)
4-8: LGTM! Consider documenting the trigger conditions?The trigger conditions are well-structured, focusing on relevant changes while excluding tests. Would it be helpful to add a comment explaining why we're watching the entire
airbyte_cdkdirectory instead of specific files? This could help future maintainers understand the decision, wdyt? 🤔
79-86: How about adding some error handling here?The Python setup looks good, but what if any of these steps fail? Maybe we could add some error checking? Here's a thought:
python -m venv .venv || exit 1 source .venv/bin/activate -pip install --upgrade pip -pip install poetry==1.6.1 +pip install --upgrade pip || exit 1 +pip install poetry==1.6.1 || exit 1What do you think? 🤔
89-119: Great error handling! Small shell scripting enhancement?The version management is robust with excellent error handling! 🎉 Just a couple of small suggestions:
- The
grepcommand could be more precise with word boundaries:-valid_cdk_version=$(echo "$cdk_version" | grep -Eo '^[0-9]+\.[0-9]+\.[0-9]+') +valid_cdk_version=$(echo "$cdk_version" | grep -Eo '^\b[0-9]+\.[0-9]+\.[0-9]+\b')
- Let's quote the PATH addition to prevent word splitting:
-export PATH="/home/runner/.local/bin:$PATH" +export PATH="/home/runner/.local/bin:${PATH}"What are your thoughts on these changes? 🤔
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/cdk-publish.yml(5 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/cdk-publish.yml
88-88: shellcheck reported issue in this script: SC2086:info:30:46: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/cdk-publish.yml (3)
44-47: LGTM! Good security practice enhancement
The SARIF format output for Trivy scan results will make it easier to track and analyze vulnerabilities over time. 👍
58-61: LGTM! Well-configured Poetry environment
The Poetry environment variables are appropriately set for CI/CD context, preventing interactive prompts and managing virtualenvs efficiently. 🎯
134-136:
Remember to address the TODO about reverting tags
I notice there's a TODO comment about reverting the tags before merging to main. Should we handle this now? Also, the test prefix in the tags suggests this might be temporary - is that intentional? 🤔
| source .venv/bin/activate | ||
| pip install --upgrade pip | ||
| pip install poetry==1.6.1 | ||
| poetry config virtualenvs.create false |
There was a problem hiding this comment.
Probably can either delete this line or the ENV variable on line 61
What
Fixes the publish flow for SDM by accounting for dynamic CDK versioning.
How
Notes