hainenber commented on code in PR #45909:
URL: https://github.com/apache/superset/pull/45909#discussion_r4444689663
##########
.github/workflows/pre-commit.yml:
##########
@@ -174,8 +174,22 @@ jobs:
uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 #
v7.1.0
with:
node-version-file: "superset-frontend/.nvmrc"
- cache: "npm"
- cache-dependency-path: "superset-frontend/package-lock.json"
+
+ # Not setup-node's built-in `cache: npm`: it keys on the exact
+ # package-lock.json hash with no fallback, so every lockfile change
+ # makes npm ci re-download every tarball (including xlsx from
+ # cdn.sheetjs.com), exposing the install to any registry/CDN outage
+ # at that moment, which is how runs 40649064300 and 40679638770
+ # failed. Restoring from the prefix instead cuts the downloads to the
+ # changed packages.
+ - name: Cache npm
+ if: steps.changed_files.outputs.needs_node == 'true'
+ uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
+ with:
+ path: ~/.npm
Review Comment:
That's a good catch. Apologies to both folks, I did miss that setup-node's
npm caching but IMO, having a second layer of caching makes eviction in case of
cache poisoning *harder*. Having said that, I'd acknowledge that I was a bit
security-paranoia since[ all workflow triggers in `pre-commit` workflow are
managed well re: cache
poisoning](https://docs.github.com/en/actions/reference/workflows-and-actions/dependency-caching#cache-access-for-low-trust-workflow-triggers).
I'd agree with @geido here, the `prek-v1` above should suffice
--
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]
