sadpandajoe commented on code in PR #45217:
URL: https://github.com/apache/superset/pull/45217#discussion_r4257693049
##########
.github/workflows/superset-e2e.yml:
##########
@@ -135,6 +144,12 @@ jobs:
node-version-file: "./superset-frontend/.nvmrc"
cache: "npm"
cache-dependency-path: "superset-frontend/package-lock.json"
+ - name: Setup Develocity for npm
Review Comment:
This workflow can be started with `workflow_dispatch` and a `pr_id`, which
checks out a fork PR's merge ref. Unlike fork `pull_request` runs, a dispatch
keeps repository secrets, and the setup action exports `DEVELOCITY_ACCESS_KEY`
to later steps; it also falls back to the original key if the short-lived token
exchange fails. A malicious npm lifecycle script in that PR could then read the
key during `npm ci`. Should the Develocity setup (here and for `setup-backend`)
be skipped when a dispatch checks out a PR or arbitrary ref?
##########
.github/workflows/superset-frontend.yml:
##########
@@ -92,10 +114,21 @@ jobs:
run: |
zstd -d < docker-image.tar.zst | docker load
+ - name: Setup Develocity for npm
+ id: develocity-npm
+ if: env.DEVELOCITY_ENABLED == 'true'
+ continue-on-error: true
+ uses: $/.github/actions/setup-develocity-npm
+ with:
+ develocity-access-key: ${{ secrets.DEVELOCITY_ACCESS_KEY }}
+
- name: npm run test with coverage
+ env:
+ DEVELOCITY_DOCKER_ENV: ${{ steps.develocity-npm.outcome == 'success'
&& env.DEVELOCITY_DOCKER_ARGS || '' }}
Review Comment:
If the npm install inside the setup action fails (registry or download
error), the action only logs a warning and the step still finishes as
`success`, so this expression still switches on the
`--require=@gradle-tech/develocity-agent/preload` `NODE_OPTIONS` for the
container. With the module missing, `npm` fails with `Cannot find module`, so a
Develocity outage can fail the job, which the continue-on-error change is meant
to prevent. The same gate is used by the other frontend jobs here and in
`chromatic.yml`. Would it make sense to also check that the agent is actually
present under `/tmp/develocity-npm` before enabling the injection?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
