Only use mirrorToken in getManifest if it's provided#1548
Conversation
|
no one should be allowed to make any changes |
|
Hi @deiga 👋 |
|
Doh! 🤦 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adjusts how authentication tokens are selected when downloading Node distributions and fetching the node-versions manifest, preferring the mirror token only when it’s explicitly present.
Changes:
- Update
tc.downloadToolauth selection to usemirrorTokenonly when bothmirrorandmirrorTokenare set. - Update
tc.getManifestFromRepoauth selection similarly. - Regenerate/update the compiled
dist/setup/index.jsto reflect the source change.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/distributions/official_builds/official_builds.ts | Changes token selection logic for downloads and manifest fetches. |
| dist/setup/index.js | Compiled output updated to match the source logic change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| downloadPath = await tc.downloadTool( | ||
| versionInfo.downloadUrl, | ||
| undefined, | ||
| this.nodeInfo.mirror ? this.nodeInfo.mirrorToken : this.nodeInfo.auth | ||
| this.nodeInfo.mirror && this.nodeInfo.mirrorToken | ||
| ? this.nodeInfo.mirrorToken | ||
| : this.nodeInfo.auth | ||
| ); |
There was a problem hiding this comment.
In the current implementation it's not possible for versionInfo.downloadUrl to be a non-github host
| return tc.getManifestFromRepo( | ||
| 'actions', | ||
| 'node-versions', | ||
| this.nodeInfo.mirror ? this.nodeInfo.mirrorToken : this.nodeInfo.auth, | ||
| this.nodeInfo.mirror && this.nodeInfo.mirrorToken | ||
| ? this.nodeInfo.mirrorToken | ||
| : this.nodeInfo.auth, | ||
| 'main' | ||
| ); |
There was a problem hiding this comment.
IMO 2 call sites doesn't call for deduplication
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
deiga
force-pushed
the
fix/only-use-mirror-token-for-manifest-if-set
branch
from
f1a144b to
bf9e4b9
Compare
Description:
When providing a
mirror, but not amirrorTokenthengetManifestwill use the emptymirrorTokeninstead of any possible github token. This can lead to API Rate Limit exhaustion inadvertently.The proposed fix is to only supply
mirrorTokenif it is actually set.Related issue:
I couldn't find any directly related issues
Check list: