Conversation
📝 WalkthroughWalkthroughThe pull request modifies the RC secure artifact S3 download path in the build workflow from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @rberezen's task in 36s —— View job PR Review: Fix S3 path for RC buildsTasks
Review FindingsLGTM - no significant issues found The change updates the S3 path for RC builds from Verified:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-qa-docker.yml (1)
326-337:⚠️ Potential issue | 🟠 MajorStale RC S3 path in
download-after-trigger— same bug, missed in the parallel path.Line 333 still uses the old
releases/rc/${VERSION}path, which is exactly what was fixed on line 174. Although this branch is currently unreachable —download-after-triggeronly executes whentrigger_liquibase_secure_workflow == 'true', and that output is only ever set wheninputs.secure-versionis empty (so theif [[ -n "${{ inputs.secure-version }}" ]]condition on line 326 is always false) — the stale URL is a latent regression: if the step guard is relaxed in a future change, it will silently hit the wrong S3 path.Apply the same fix here for consistency:
🐛 Proposed fix
if [[ "$VERSION" =~ ^([0-9]+\.[0-9]+\.[0-9]+)-RC([0-9]+)$ ]]; then BASE_VERSION="${BASH_REMATCH[1]}" RC_NUMBER="${BASH_REMATCH[2]}" LIQUIBASE_SECURE_ZIP="liquibase-secure-${VERSION}.tar.gz" - S3_URL="s3://repo.liquibase.com/releases/rc/${VERSION}/${LIQUIBASE_SECURE_ZIP}" + S3_URL="s3://repo.liquibase.com/non-releases/secure/${VERSION}/${LIQUIBASE_SECURE_ZIP}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-qa-docker.yml around lines 326 - 337, Stale RC S3 path: in the branch matching the RC regex for inputs.secure-version, update the LIQUIBASE_SECURE_ZIP/S3_URL assignment to mirror the fix applied earlier by using BASE_VERSION and RC_NUMBER (not the raw VERSION) and the corrected releases/rc path; specifically change the S3_URL construction in that RC branch so it references releases/rc/${BASE_VERSION}-RC${RC_NUMBER}/${LIQUIBASE_SECURE_ZIP} (and adjust LIQUIBASE_SECURE_ZIP if your earlier fix altered its name) to ensure the RC upload/download path is consistent with the prior fix.
🧹 Nitpick comments (1)
.github/workflows/build-qa-docker.yml (1)
171-172:BASE_VERSIONandRC_NUMBERare captured but never used.These variables are extracted on lines 171–172 (and again on 330–331 in
download-after-trigger) but are not referenced anywhere in either step. Remove them to reduce noise, or document their intended future use.🧹 Proposed cleanup (apply in both download steps)
if [[ "$VERSION" =~ ^([0-9]+\.[0-9]+\.[0-9]+)-RC([0-9]+)$ ]]; then - BASE_VERSION="${BASH_REMATCH[1]}" - RC_NUMBER="${BASH_REMATCH[2]}" LIQUIBASE_SECURE_ZIP="liquibase-secure-${VERSION}.tar.gz" S3_URL="s3://repo.liquibase.com/non-releases/secure/${VERSION}/${LIQUIBASE_SECURE_ZIP}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-qa-docker.yml around lines 171 - 172, Remove the unused variable assignments BASE_VERSION and RC_NUMBER from the download steps: delete the two lines that set BASE_VERSION="${BASH_REMATCH[1]}" and RC_NUMBER="${BASH_REMATCH[2]}" (and the duplicate occurrences in the other download-after-trigger step) so the workflow no longer captures unused values; if you intend to keep them for future use instead, add a comment explaining their planned purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/build-qa-docker.yml:
- Around line 326-337: Stale RC S3 path: in the branch matching the RC regex for
inputs.secure-version, update the LIQUIBASE_SECURE_ZIP/S3_URL assignment to
mirror the fix applied earlier by using BASE_VERSION and RC_NUMBER (not the raw
VERSION) and the corrected releases/rc path; specifically change the S3_URL
construction in that RC branch so it references
releases/rc/${BASE_VERSION}-RC${RC_NUMBER}/${LIQUIBASE_SECURE_ZIP} (and adjust
LIQUIBASE_SECURE_ZIP if your earlier fix altered its name) to ensure the RC
upload/download path is consistent with the prior fix.
---
Nitpick comments:
In @.github/workflows/build-qa-docker.yml:
- Around line 171-172: Remove the unused variable assignments BASE_VERSION and
RC_NUMBER from the download steps: delete the two lines that set
BASE_VERSION="${BASH_REMATCH[1]}" and RC_NUMBER="${BASH_REMATCH[2]}" (and the
duplicate occurrences in the other download-after-trigger step) so the workflow
no longer captures unused values; if you intend to keep them for future use
instead, add a comment explaining their planned purpose.
|
Claude finished @jandroav's task in 1m 7s —— View job PR Review: Fix S3 path for RC buildsTasks
Review FindingsThe fix at line 174 correctly updates the initial download path from Issue: Stale RC S3 path in Line 333 in the # Line 333 - still uses old path:
S3_URL="s3://repo.liquibase.com/releases/rc/${VERSION}/${LIQUIBASE_SECURE_ZIP}"
# Should be:
S3_URL="s3://repo.liquibase.com/non-releases/secure/${VERSION}/${LIQUIBASE_SECURE_ZIP}"This step is currently unreachable with Minor: |
No description provided.