fix(ci): improve ref derivation in docker publish workflow #3715
Conversation
Replaces reliance on github.event_name with explicit input and payload
checks. This ensures the workflow correctly identifies release intent
when called via workflow_call, as event_name is inherited from the
caller ('push' in our case, because the changesets_release.yml workflow
is triggered by a push to main).
📝 WalkthroughWalkthroughReplaces event-name branching in the Docker Hub sync workflow with a linear if-elif-else chain that sets Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (6)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3715 +/- ##
==========================================
- Coverage 87.36% 87.31% -0.05%
==========================================
Files 23 23
Lines 2011 2011
Branches 532 530 -2
==========================================
- Hits 1757 1756 -1
- Misses 252 253 +1
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/sync_service_dockerhub_image.yml (1)
159-167: Consider using env vars here too for consistency.While these values come from job outputs rather than direct user input (lower risk), using the same safe pattern with environment variables would be more defensive and consistent with the fix above.
♻️ Suggested improvement
- name: Derive image tags from the GitHub Actions event + env: + IS_RELEASE: ${{ needs.derive_build_vars.outputs.is_release }} + ELECTRIC_VERSION: ${{ needs.derive_build_vars.outputs.electric_version }} + SHORT_SHA: ${{ needs.derive_build_vars.outputs.short_commit_sha }} run: | - if [ "${{ needs.derive_build_vars.outputs.is_release }}" = "true" ]; then + if [ "$IS_RELEASE" = "true" ]; then # A release triggers official release image publishing - echo "ELECTRIC_TAGS=-t $DOCKERHUB_REPO:latest -t $DOCKERHUB_REPO:${{ needs.derive_build_vars.outputs.electric_version }}" >> $GITHUB_ENV + echo "ELECTRIC_TAGS=-t $DOCKERHUB_REPO:latest -t $DOCKERHUB_REPO:$ELECTRIC_VERSION" >> $GITHUB_ENV echo "ELECTRIC_CANARY_TAGS=" >> $GITHUB_ENV else # A regular push to the main branch triggers canary image publishing echo "ELECTRIC_TAGS=-t $DOCKERHUB_REPO:canary" >> $GITHUB_ENV - echo "ELECTRIC_CANARY_TAGS=-t $DOCKERHUB_CANARY_REPO:latest -t $DOCKERHUB_CANARY_REPO:${{ needs.derive_build_vars.outputs.short_commit_sha }}" >> $GITHUB_ENV + echo "ELECTRIC_CANARY_TAGS=-t $DOCKERHUB_CANARY_REPO:latest -t $DOCKERHUB_CANARY_REPO:$SHORT_SHA" >> $GITHUB_ENV fi
🤖 Fix all issues with AI agents
In @.github/workflows/sync_service_dockerhub_image.yml:
- Around line 43-52: The workflow currently interpolates `${{ inputs.release_tag
}}` and `${{ github.event.release.tag_name }}` directly into the shell script
(setting `ref` and `is_release`), which risks command injection; change the
job/step to pass those values as environment variables (e.g., set env:
RELEASE_TAG: ${{ inputs.release_tag }} and env: GITHUB_RELEASE_TAG: ${{
github.event.release.tag_name }}) and then in the shell use safe, quoted POSIX
parameter checks like `if [ -n "$RELEASE_TAG" ]; then
ref="refs/tags/$RELEASE_TAG"; is_release=true; elif [ -n "$GITHUB_RELEASE_TAG"
]; then ref="refs/tags/$GITHUB_RELEASE_TAG"; is_release=true; else
ref="$GITHUB_SHA"; is_release=false; fi` so no workflow expressions are injected
directly into the script and all expansions are quoted.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/sync_service_dockerhub_image.yml
⏰ 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). (6)
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Run Lux integration tests
🔇 Additional comments (1)
.github/workflows/sync_service_dockerhub_image.yml (1)
4-4: LGTM!Comment accurately reflects the updated location of trigger-dependent logic.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/sync_service_dockerhub_image.yml (1)
161-171: Consider applying the same environment variable pattern here for consistency.While the values interpolated here (
is_release,electric_version,short_commit_sha) are derived from controlled sources (step outputs and git metadata), using environment variables would maintain consistency with the security-conscious approach applied in thederive_build_varsjob.♻️ Optional: Environment variable approach for consistency
- name: Derive image tags from the GitHub Actions event + env: + IS_RELEASE: ${{ needs.derive_build_vars.outputs.is_release }} + ELECTRIC_VERSION: ${{ needs.derive_build_vars.outputs.electric_version }} + SHORT_COMMIT_SHA: ${{ needs.derive_build_vars.outputs.short_commit_sha }} run: | - if [ "${{ needs.derive_build_vars.outputs.is_release }}" = "true" ]; then + if [ "$IS_RELEASE" = "true" ]; then # A release triggers official release image publishing - echo "ELECTRIC_TAGS=-t $DOCKERHUB_REPO:latest -t $DOCKERHUB_REPO:${{ needs.derive_build_vars.outputs.electric_version }}" >> $GITHUB_ENV + echo "ELECTRIC_TAGS=-t $DOCKERHUB_REPO:latest -t $DOCKERHUB_REPO:$ELECTRIC_VERSION" >> $GITHUB_ENV echo "ELECTRIC_CANARY_TAGS=" >> $GITHUB_ENV else # A regular push to the main branch triggers canary image publishing echo "ELECTRIC_TAGS=-t $DOCKERHUB_REPO:canary" >> $GITHUB_ENV - echo "ELECTRIC_CANARY_TAGS=-t $DOCKERHUB_CANARY_REPO:latest -t $DOCKERHUB_CANARY_REPO:${{ needs.derive_build_vars.outputs.short_commit_sha }}" >> $GITHUB_ENV + echo "ELECTRIC_CANARY_TAGS=-t $DOCKERHUB_CANARY_REPO:latest -t $DOCKERHUB_CANARY_REPO:$SHORT_COMMIT_SHA" >> $GITHUB_ENV fi
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/sync_service_dockerhub_image.yml
⏰ 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). (6)
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Run Lux integration tests
🔇 Additional comments (2)
.github/workflows/sync_service_dockerhub_image.yml (2)
4-4: LGTM!Comment accurately points maintainers to the correct location for conditional logic.
40-59: LGTM! Well-structured fix for the ref derivation logic.The refactored approach correctly:
- Uses environment variables to safely handle inputs (addressing the script injection concern)
- Prioritizes
inputs.release_tagovergithub.event.release.tag_name, fixing the root cause whereworkflow_calltriggers inherit the caller'sevent_name- Uses idiomatic
-nchecks for non-empty strings
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
🔗 Context
When this workflow is called by the Changesets workflow, the
github.event_namecontext variable evaluates topush(the parent's trigger) rather thanworkflow_call. This caused our previous logic to skip the release flow and default to canary builds.So the most recent package publishing only pushed canary images:
🛠️ Changes
derive_build_varsjob to prioritizeinputs.release_tagandgithub.event.release.tag_nameover the event name string.-n(non-zero string) checks.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.