ci: create unified cdk, sdm, and builder publish flow #107
Conversation
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request updates two GitHub workflow files: Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Wdyt? 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: 3
🧹 Outside diff range and nitpick comments (5)
.github/workflows/publish_sdm_connector.yml (4)
3-3: Consider consolidating the workflows sooner.Since this workflow is marked for deletion and duplicates functionality from
pypi_publish.yml, would it make sense to prioritize the consolidation to reduce maintenance overhead? The duplication increases the risk of divergent implementations. wdyt?
Line range hint
33-70: Enhance shell script robustness.The version validation logic looks solid! However, would you consider adding double quotes around variables to prevent potential word splitting and globbing issues? For example:
-if [ -z "$VERSION" ]; then +if [ -z "${VERSION}" ]; thenThis makes the script more robust against edge cases. wdyt?
Line range hint
161-165: Consider caching Docker layers.The multi-platform build setup looks great! Would you consider adding Docker layer caching to speed up builds? You can add:
- name: Set up Docker Buildx uses: docker/setup-buildx-action@v3 + with: + buildkitd-flags: --debug + driver-opts: | + image=moby/buildkit:latest + network=hostThis could significantly reduce build times. wdyt?
Line range hint
167-171: Consider using GITHUB_TOKEN for container registry.Have you considered using GitHub Container Registry (ghcr.io) instead of DockerHub? It would allow you to use the built-in
GITHUB_TOKENinstead of managing separate credentials, plus it integrates better with GitHub's security features. wdyt?.github/workflows/pypi_publish.yml (1)
16-30: Consider enhancing input documentation.The new input parameters are great for control! Would you consider adding examples and use-cases to the descriptions? For instance:
publish_to_pypi: - description: "Publish to PyPI. If true, the workflow will publish to PyPI." + description: | + "Publish to PyPI. If true, the workflow will publish to PyPI. + Example: Use 'false' when you only want to publish to DockerHub."This would make it clearer when to use each option. wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/publish_sdm_connector.yml(1 hunks).github/workflows/pypi_publish.yml(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pypi_publish.yml
38-38: shellcheck reported issue in this script: SC2086:info:6:48: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:6:22: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:24:30: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:25:30: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:28:33: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:29:33: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:31:32: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
233-233: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
237-237: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
242-242: shellcheck reported issue in this script: SC2086:info:5:6: Double quote to prevent globbing and word splitting
(shellcheck)
273-273: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
302-302: if: condition "${{ failure() }} && 1 == 0" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
311-311: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pypi_publish.yml (2)
36-81: Consider enhancing shell script robustness.The version detection and validation logic looks solid! Would you consider adding some shell safety features? wdyt?
- name: Validate and set VERSION run: | + set -euo pipefail INPUT_VERSION=${{ github.event.inputs.version }} - echo "Version input set to '${INPUT_VERSION}'" + echo "Version input set to '${INPUT_VERSION:-none}'" # Exit with success if both detected and input versions are empty - if [ -z "${DETECTED_VERSION:-}" ] && [ -z "${INPUT_VERSION:-}" ]; then + if [ -z "${DETECTED_VERSION:-}" ] && [ -z "${INPUT_VERSION:-}" ]; then🧰 Tools
🪛 actionlint (1.7.4)
38-38: shellcheck reported issue in this script: SC2086:info:6:48: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:6:22: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:24:30: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:25:30: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:28:33: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:29:33: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:31:32: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
242-251: Consider using requirements.txt for dependency management.The current approach of using
pip-toolsin the workflow might be more robust with a pre-committedrequirements.txt. Would you consider this alternative approach? wdyt?- - name: Update Builder's CDK version run: | PREVIOUS_VERSION=$(cat oss/airbyte-connector-builder-resources/CDK_VERSION) sed -i "s/${PREVIOUS_VERSION}/${VERSION}/g" oss/airbyte-connector-builder-server/Dockerfile sed -i "s/${PREVIOUS_VERSION}/${VERSION}/g" cloud/airbyte-connector-builder-server-wrapped/Dockerfile - sed -i "s/airbyte-cdk==${PREVIOUS_VERSION}/airbyte-cdk==${VERSION}/g" oss/airbyte-connector-builder-server/requirements.in + sed -i "s/airbyte-cdk==${PREVIOUS_VERSION}/airbyte-cdk==${VERSION}/g" oss/airbyte-connector-builder-server/requirements.txt echo ${VERSION} > oss/airbyte-connector-builder-resources/CDK_VERSION - cd oss/airbyte-connector-builder-server - pip install pip-tools - pip-compile --upgrade🧰 Tools
🪛 actionlint (1.7.4)
242-242: shellcheck reported issue in this script: SC2086:info:5:6: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pypi_publish.yml(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pypi_publish.yml
38-38: shellcheck reported issue in this script: SC2086:info:6:48: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:6:22: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:24:30: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:25:30: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:28:33: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:29:33: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:31:32: Double quote to prevent globbing and word splitting
(shellcheck)
48-48: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
242-242: shellcheck reported issue in this script: SC2086:info:5:6: Double quote to prevent globbing and word splitting
(shellcheck)
273-273: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
302-302: if: condition "${{ failure() }} && 1 == 0" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
311-311: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
🔇 Additional comments (4)
.github/workflows/pypi_publish.yml (4)
14-30: LGTM! Well-structured input parameters.
The input parameters are well-documented and provide good control over the publishing process. The default values make sense for the intended workflow.
301-337: Consider simplifying the Slack notification conditions.
The current approach uses 1 == 0 to disable notifications. Would you consider using a proper feature flag instead? This was previously mentioned in past reviews, but remains unaddressed. wdyt?
🧰 Tools
🪛 actionlint (1.7.4)
302-302: if: condition "${{ failure() }} && 1 == 0" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
311-311: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
264-299:
Add missing changelog-message input parameter.
The Slack notification setup references an undefined changelog-message parameter. Would you consider adding it to the workflow inputs? wdyt?
on:
workflow_dispatch:
inputs:
+ changelog-message:
+ description: "Changelog message for Slack notifications"
+ type: string
+ required: false
version:
description: "Version to publish"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
273-273: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
133-220: Verify Docker image compatibility.
The SDM publishing job looks well-structured, but there's a potential concern about platform compatibility.
✅ Verification successful
Multi-platform build setup looks good! 🚀
The Docker setup is well-configured for multi-platform builds:
- Uses a standard Python base image that supports both amd64 and arm64
- No platform-specific dependencies or commands in the Dockerfile
- Build configuration properly sets up QEMU and Buildx for multi-arch builds
- Dependencies in pyproject.toml are platform-agnostic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Dockerfile has any platform-specific dependencies
# that might conflict with multi-platform builds
# Look for platform-specific commands or dependencies
rg -i '(apt|apk|yum|dnf|rpm|dpkg|arch|uname)' Dockerfile
# Check for any CPU architecture specific instructions
rg -i '(x86|amd64|arm64|arm|intel|aarch)' Dockerfile
Length of output: 113
Script:
#!/bin/bash
# Let's check if Dockerfile exists and its contents
fd Dockerfile --type f --exec cat {}
# Also check for any build arguments or environment variables that might affect platform compatibility
fd Dockerfile --type f --exec grep -i 'FROM\|ARG\|ENV' {}
Length of output: 1696
Script:
#!/bin/bash
# Let's check the base image's platform support
docker manifest inspect docker.io/airbyte/python-connector-base:2.0.0 2>/dev/null | grep -i "platform\|architecture"
# Also check if there are any Python packages with platform-specific dependencies
cat pyproject.toml 2>/dev/null || echo "pyproject.toml not found"
Length of output: 8684
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
.github/workflows/pypi_publish.yml (5)
16-30: Consider safer defaults for workflow inputs?Currently, all boolean inputs default to
true. This means if someone triggers the workflow manually without explicitly setting these values, it will perform all operations. Would you consider defaulting these tofalsefor a more conservative approach? This would require users to explicitly opt-in to these operations. wdyt?publish_to_pypi: type: boolean required: true - default: true + default: false publish_to_dockerhub: type: boolean required: true - default: true + default: false update_connector_builder: type: boolean required: true - default: true + default: false
37-82: Enhance shell script robustness?The version detection and validation logic looks solid! Would you consider adding these improvements to prevent potential issues with word splitting and unquoted variables? wdyt?
if [ -z "${DETECTED_VERSION:-}" ] && [ -z "${INPUT_VERSION:-}" ]; then - echo "No version detected or input. Will publish to SHA tag instead." - echo 'VERSION=' >> $GITHUB_ENV + echo "No version detected or input. Will publish to SHA tag instead." + echo 'VERSION=' >> "$GITHUB_ENV" exit 0 fi🧰 Tools
🪛 actionlint (1.7.4)
39-39: shellcheck reported issue in this script: SC2086:info:6:48: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:6:22: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:24:30: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:25:30: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:28:33: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:29:33: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:31:32: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
111-114: Consider improving condition readability?The publishing condition is correct but quite complex. Would you consider breaking it down using variables for better readability? wdyt?
- if: (github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v')) || (github.event_name == 'workflow_dispatch' && (github.event.inputs.publish_to_pypi == 'true' || github.event.inputs.update_connector_builder == 'true')) + if: | + ( + github.event_name == 'push' && + startsWith(github.ref, 'refs/tags/v') + ) || ( + github.event_name == 'workflow_dispatch' && ( + github.event.inputs.publish_to_pypi == 'true' || + github.event.inputs.update_connector_builder == 'true' + ) + )
137-141: Track the pending automation task?There's a TODO comment about automating the publishing process after each release, blocked by issue #64. Would you like me to create a GitHub issue to track this task?
175-189: LGTM with a minor suggestionThe Docker tag verification logic looks good! However, would you consider adding quotes around the tag variable to prevent potential issues with special characters? wdyt?
- if DOCKER_CLI_EXPERIMENTAL=enabled docker manifest inspect "$tag" > /dev/null 2>&1; then + if DOCKER_CLI_EXPERIMENTAL=enabled docker manifest inspect "${tag}" > /dev/null 2>&1; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pypi_publish.yml(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pypi_publish.yml
39-39: shellcheck reported issue in this script: SC2086:info:6:48: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:6:22: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:24:30: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:25:30: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:28:33: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:29:33: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:31:32: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
245-245: shellcheck reported issue in this script: SC2086:info:5:6: Double quote to prevent globbing and word splitting
(shellcheck)
276-276: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
305-305: if: condition "${{ failure() }} && 1 == 0" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
314-314: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
🔇 Additional comments (3)
.github/workflows/pypi_publish.yml (3)
97-100: LGTM!
The job outputs are well-defined and properly used by downstream jobs.
245-254: Enhance shell script robustness?
The version update logic looks good! Would you consider adding these improvements? wdyt?
+set -euo pipefail
-PREVIOUS_VERSION=$(cat oss/airbyte-connector-builder-resources/CDK_VERSION)
+PREVIOUS_VERSION="$(cat oss/airbyte-connector-builder-resources/CDK_VERSION)"
-pip install pip-tools
+python -m pip install --no-cache-dir pip-tools🧰 Tools
🪛 actionlint (1.7.4)
245-245: shellcheck reported issue in this script: SC2086:info:5:6: Double quote to prevent globbing and word splitting
(shellcheck)
267-302:
Add missing changelog-message input parameter?
The Slack notification uses changelog-message but this parameter isn't defined in the workflow inputs. Would you consider adding it? wdyt?
inputs:
+ changelog-message:
+ description: "Changelog message for Slack notifications"
+ type: string
+ required: falseLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
276-276: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
.github/workflows/pypi_publish.yml (4)
37-82: Consider enhancing shell script robustness.Would you consider adding quotes around variable expansions to prevent potential issues with special characters? Here's a suggestion:
- DETECTED_VERSION=${{ github.ref_name }} + DETECTED_VERSION="${{ github.ref_name }}" - echo "Version ref set to '${DETECTED_VERSION}'" + echo "Version ref set to '${DETECTED_VERSION:?}'" - VERSION="${INPUT_VERSION:-$DETECTED_VERSION}" + VERSION="${INPUT_VERSION:-${DETECTED_VERSION}}"This makes the script more robust against special characters and unset variables. wdyt?
🧰 Tools
🪛 actionlint (1.7.4)
39-39: shellcheck reported issue in this script: SC2086:info:6:48: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:6:22: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:24:30: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:25:30: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:28:33: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:29:33: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:31:32: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
115-115: Consider improving condition readability.The publishing conditions are quite complex. Would you consider extracting them into reusable expressions? For example:
env: IS_RELEASE_TAG: ${{ github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v') }} SHOULD_PUBLISH_PYPI: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.publish_to_pypi == 'true' }} if: ${{ env.IS_RELEASE_TAG || env.SHOULD_PUBLISH_PYPI }}This would make the conditions more maintainable and easier to understand. wdyt?
Also applies to: 136-136, 141-145
179-193: Consider enhancing version tag validation.The version tag check is good, but would you consider adding a regex validation for semantic versioning? For example:
if [ -z "$tag" ]; then echo "Error: VERSION is not set. Ensure the tag follows the format 'refs/tags/vX.Y.Z'." exit 1 fi +if ! [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9]+)?$ ]]; then + echo "Error: Version '$VERSION' does not follow semantic versioning format." + exit 1 +fiThis would catch invalid version formats early. wdyt?
255-263: Enhance shell script robustness.Would you consider adding error handling and safer file operations? For example:
+set -euo pipefail + +if [ ! -f "oss/airbyte-connector-builder-resources/CDK_VERSION" ]; then + echo "Error: CDK_VERSION file not found" + exit 1 +fi + PREVIOUS_VERSION=$(cat oss/airbyte-connector-builder-resources/CDK_VERSION) -sed -i "s/${PREVIOUS_VERSION}/${VERSION}/g" oss/airbyte-connector-builder-server/Dockerfile +sed -i "s/${PREVIOUS_VERSION}/${VERSION}/g" "oss/airbyte-connector-builder-server/Dockerfile" cd oss/airbyte-connector-builder-server -pip install pip-tools +python -m pip install --no-cache-dir pip-toolsThis adds better error handling and more robust command execution. wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pypi_publish.yml(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pypi_publish.yml
39-39: shellcheck reported issue in this script: SC2086:info:6:48: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:6:22: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:24:30: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:25:30: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:28:33: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:29:33: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:31:32: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
286-286: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
315-315: if: condition "${{ failure() }} && 1 == 0" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
324-324: property "changelog-message" is not defined in object type {publish_to_dockerhub: string; publish_to_pypi: string; update_connector_builder: string; version: string}
(expression)
🔇 Additional comments (2)
.github/workflows/pypi_publish.yml (2)
14-30: LGTM! Well-structured input parameters.
The input parameters are clearly described and provide good control over the workflow's behavior.
101-103: LGTM! Well-defined job outputs.
The outputs are properly defined and used consistently throughout the workflow.
…ckage validation to pytest-fast
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.github/workflows/pypi_publish.yml (4)
40-85: Consider enhancing shell script robustness.The version detection and validation logic looks good! Would you consider adding these improvements?
run: | + set -euo pipefail DETECTED_VERSION=${{ github.ref_name }} - echo "Version ref set to '${DETECTED_VERSION}'" + echo "Version ref set to '${DETECTED_VERSION:?}'"This adds better error handling and ensures variables are properly quoted. wdyt?
🧰 Tools
🪛 actionlint (1.7.4)
42-42: shellcheck reported issue in this script: SC2086:info:6:48: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:6:22: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:24:30: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:25:30: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:28:33: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:29:33: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:31:32: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
142-148: Document the timeline for enabling automatic releases.The TODO comment indicates a future plan to enable automatic releases after each tag. Would you consider adding a target date or milestone to the comment? This would help track when this change should be implemented. wdyt?
182-196: Enhance Docker tag existence check.The tag existence check is good! Would you consider adding retries for network resilience?
run: | + set -euo pipefail + max_attempts=3 + attempt=1 + while [ $attempt -le $max_attempts ]; do 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 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 + break + doneThis would help handle temporary DockerHub API issues. wdyt?
259-268: Enhance version update robustness.The version update steps look good! Would you consider these improvements?
command: | + set -euo pipefail PREVIOUS_VERSION=$(cat oss/airbyte-connector-builder-resources/CDK_VERSION) - sed -i "s/${PREVIOUS_VERSION}/${VERSION}/g" oss/airbyte-connector-builder-server/Dockerfile + sed -i "s/${PREVIOUS_VERSION:?}/${VERSION:?}/g" "oss/airbyte-connector-builder-server/Dockerfile" cd oss/airbyte-connector-builder-server - pip install pip-tools + python -m pip install --no-cache-dir pip-toolsThis adds better error handling and more robust command execution. wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/pypi_publish.yml(4 hunks).github/workflows/pytest_fast.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pypi_publish.yml
42-42: shellcheck reported issue in this script: SC2086:info:6:48: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:6:22: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:24:30: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:25:30: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:28:33: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:29:33: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:31:32: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:32:32: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/pytest_fast.yml (1)
10-28: LGTM! Well-structured build job addition.
The new test-build job looks good! It:
- Uses the recommended
hynek/build-and-inspect-python-packageaction - Properly handles dynamic versioning
- Uploads artifacts with a unique name using the run ID
.github/workflows/pypi_publish.yml (1)
280-282: Replace hardcoded condition with proper feature flag.
Instead of using 1 == 0, would you consider adding a proper input parameter for controlling Slack notifications? For example:
inputs:
enable_slack_notifications:
description: "Enable Slack notifications"
type: boolean
required: true
default: falseThen use it in conditions:
if: ${{ inputs.enable_slack_notifications == true }}This would make it easier to enable/disable notifications. wdyt?
aaronsteers
deleted the
aj/ci/make-unified-publish-workflow-for-sdm-and-cdk
branch
This brings over logic from the SDM docker image publish flow, and consolidates these four tasks:
publish_to_pypi==Truein workflow dispatch.publish_to_dockerhub==True.--use-local-cdkor similar) #64 ships, we will additionally run this on all release tags.update_connector_builder==True.Screenshot of workflow dispatch prompts:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Documentation