feat(ci): remove redundant dependency:go-offline (DAT-22570)#516
feat(ci): remove redundant dependency:go-offline (DAT-22570)#516
Conversation
…AT-22570) Remove dependency:go-offline goal from build commands in pro-extension-build-for-liquibase and pro-extension-test workflows. With stickydisk/actions-cache providing warm Maven caches, the pre-fetch step is redundant and adds unnecessary overhead.
📝 WalkthroughWalkthroughThis PR removes the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| run: | | ||
| if [ -n "${{ inputs.extraMavenArgs }}" ]; then | ||
| mvn -B dependency:go-offline clean package -DskipTests=true "${{ inputs.extraMavenArgs }}" | ||
| mvn -B clean package -DskipTests=true "${{ inputs.extraMavenArgs }}" |
Check warning
Code scanning / CodeQL
Code injection Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
General fix: Avoid using ${{ ... }} expressions directly in run: command bodies for untrusted inputs. Instead, assign the input to an environment variable via the env: section, and reference it using shell variable syntax (e.g., $EXTRA_MAVEN_ARGS) so that the shell receives the value as data, not parsed script.
Best concrete fix here, without changing behavior:
- Add an environment variable (e.g.,
EXTRA_MAVEN_ARGS) in the “Build and Package” step and set it to${{ inputs.extraMavenArgs }}. - In the
run:block, replace${{ inputs.extraMavenArgs }}with$EXTRA_MAVEN_ARGSand keep the conditionalif [ -n "$EXTRA_MAVEN_ARGS" ]; then ...check using standard bash syntax. - Do not alter any other steps or logic.
Concretely in .github/workflows/pro-extension-test.yml around lines 246–260:
- Under
env:, addEXTRA_MAVEN_ARGS: ${{ inputs.extraMavenArgs }}. - In the bash script, change the condition to test
"$EXTRA_MAVEN_ARGS"and pass"$EXTRA_MAVEN_ARGS"tomvninstead of"${{ inputs.extraMavenArgs }}".
No additional methods or external libraries are needed; this is pure YAML + shell variable usage.
| @@ -252,9 +252,10 @@ | ||
| AZURE_CLIENT_SECRET: ${{ env.AZURE_CLIENT_SECRET }} | ||
| AZURE_CLIENT_ID: ${{ env.AZURE_CLIENT_ID }} | ||
| LIQUIBASE_AZURE_STORAGE_ACCOUNT: ${{ env.LIQUIBASE_AZURE_STORAGE_ACCOUNT }} | ||
| EXTRA_MAVEN_ARGS: ${{ inputs.extraMavenArgs }} | ||
| run: | | ||
| if [ -n "${{ inputs.extraMavenArgs }}" ]; then | ||
| mvn -B clean package -DskipTests=true "${{ inputs.extraMavenArgs }}" | ||
| if [ -n "$EXTRA_MAVEN_ARGS" ]; then | ||
| mvn -B clean package -DskipTests=true "$EXTRA_MAVEN_ARGS" | ||
| else | ||
| mvn -B clean package -DskipTests=true | ||
| fi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/pro-extension-test.yml (1)
246-260:⚠️ Potential issue | 🟠 MajorSecurity: Potential command injection via direct input interpolation.
The CodeQL analysis flagged line 257. Using
${{ inputs.extraMavenArgs }}directly in the shell command is vulnerable to command injection if the input can be controlled by an external user (e.g., viaworkflow_dispatchor a malicious PR triggering aworkflow_call).Compare with
pro-extension-build-for-liquibase.yml(line 219-221), which safely passes the input through an environment variable first:env: EXTRA_MAVEN_ARGS: ${{ inputs.extraMavenArgs }} run: | mvn -B clean package -DskipTests=true ${EXTRA_MAVEN_ARGS}🛡️ Proposed fix: Use environment variable instead of direct interpolation
- name: Build and Package if: ${{ !inputs.nightly }} shell: bash env: LIQUIBASE_PRO_LICENSE_KEY: ${{ env.PRO_LICENSE_KEY }} AZURE_TENANT_ID: ${{ env.AZURE_TENANT_ID }} AZURE_CLIENT_SECRET: ${{ env.AZURE_CLIENT_SECRET }} AZURE_CLIENT_ID: ${{ env.AZURE_CLIENT_ID }} LIQUIBASE_AZURE_STORAGE_ACCOUNT: ${{ env.LIQUIBASE_AZURE_STORAGE_ACCOUNT }} + EXTRA_MAVEN_ARGS: ${{ inputs.extraMavenArgs }} run: | - if [ -n "${{ inputs.extraMavenArgs }}" ]; then - mvn -B clean package -DskipTests=true "${{ inputs.extraMavenArgs }}" + if [ -n "${EXTRA_MAVEN_ARGS}" ]; then + mvn -B clean package -DskipTests=true ${EXTRA_MAVEN_ARGS} else mvn -B clean package -DskipTests=true fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pro-extension-test.yml around lines 246 - 260, The workflow step "Build and Package" currently interpolates inputs.extraMavenArgs directly into the shell run command (risk of command injection); change it to set an environment variable (e.g., EXTRA_MAVEN_ARGS: ${{ inputs.extraMavenArgs }}) in the step env block and reference that env var inside the run script (use ${EXTRA_MAVEN_ARGS}) instead of ${ { inputs.extraMavenArgs } }; keep the existing conditional logic that checks for a non-empty value but perform the check against ${EXTRA_MAVEN_ARGS} so the input is never directly injected into the shell command.
🤖 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/pro-extension-test.yml:
- Around line 246-260: The workflow step "Build and Package" currently
interpolates inputs.extraMavenArgs directly into the shell run command (risk of
command injection); change it to set an environment variable (e.g.,
EXTRA_MAVEN_ARGS: ${{ inputs.extraMavenArgs }}) in the step env block and
reference that env var inside the run script (use ${EXTRA_MAVEN_ARGS}) instead
of ${ { inputs.extraMavenArgs } }; keep the existing conditional logic that
checks for a non-empty value but perform the check against ${EXTRA_MAVEN_ARGS}
so the input is never directly injected into the shell command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d83c324-0092-46bc-8366-4a96f6204b4f
📒 Files selected for processing (2)
.github/workflows/pro-extension-build-for-liquibase.yml.github/workflows/pro-extension-test.yml
3 participants
Summary
Removes the
dependency:go-offlineMaven goal from build commands in reusable extension workflows. With stickydisk and actions/cache providing warm Maven repository caches, the pre-fetch step is redundant overhead (~1-2 min per build per OS).Changes
pro-extension-build-for-liquibase.yml: Removeddependency:go-offlinefrom the "Build and Package" step (used by RC Release Orchestrator)pro-extension-test.yml: Removeddependency:go-offlinefrom 3 build steps (nightly build, regular build with/without extra Maven args)Companion PR
DAT-22570/optimize-rc-release-orchestrator): main pipeline optimization changes (Maven cache on Win/macOS, Docker parallelization, combined version commands)Test plan
dependency:go-offline🔗 DAT-22570