-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
download from node-versions and fallback to node dist #147
Conversation
if (toolPath) { | ||
console.log(`Found in cache @ ${toolPath}`); | ||
} else { | ||
console.log(`Attempting to download ${versionSpec}...`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added much more output to the logs. The previous impl was pretty much sllent
src/installer.ts
Outdated
let info = await getInfoFromManifest(versionSpec, stable, token); | ||
if (!info) { | ||
console.log( | ||
'Not found in manifest. Falling back to download directly from Node' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LTS lineages (8,10,12,14) will come from manifest while non-LTS/old versions will still fall through to node dist. We can choose to pull more into manifest if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compute is also going to do separate changes so the latest of each LTS is pre-cached on the image as part of image gen
src/installer.ts
Outdated
downloadPath = await tc.downloadTool(info.downloadUrl, undefined, token); | ||
} catch (err) { | ||
if (err instanceof tc.HTTPError && err.httpStatusCode == 404) { | ||
await acquireNodeFromFallbackLocation(info.resolvedVersion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few really old versions like 0.12.8 will have a different layout without a targz or 7z. it will have just a node binary
let _7zPath = path.join(__dirname, '..', 'externals', '7zr.exe'); | ||
extPath = await tc.extract7z(downloadPath, undefined, _7zPath); | ||
// 7z extracts to folder matching file name | ||
let nestedPath = path.join(extPath, path.basename(info.fileName, '.7z')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node dist creates a 7z with a nested folder matching the filename in the unzipped structure. The manifest extracts without that. That's a good thing because later I'm considering extracting directly into the cache instead of unzip and copy. Copy on windows consumes the vast majority of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like unzip + copy may be related to the extra folder after unzipping.
Move is unsafe on Windows because defender commonly still has a lock reading the newly created files on disk. Copy is safe.
extPath = await tc.extractTar(downloadPath, undefined, [ | ||
'xz', | ||
'--strip', | ||
'1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. strip 1 will strip the root folder if it exists. I validated it won't if it doesn't. Note that I extended the interface to accept string | string[] which should be fine for compat.
src/installer.ts
Outdated
'actions', | ||
'node-versions', | ||
token, | ||
'update-versions-manifest-file' // TODO: remove after testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove after Maxim merges his last set of changes today. That's the branch to pull it from and after removing it will default to master.
return null; | ||
} | ||
|
||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all mostly the same code - just moved into another function for fallback.
import * as installer from './installer'; | ||
import * as auth from './authutil'; | ||
import * as path from 'path'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved logic into a main for testing. Mostly the same logic.
// | ||
let version = core.getInput('node-version'); | ||
if (!version) { | ||
version = core.getInput('version'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even though this is officially deprecated, I left it in because I wasn't in the mood to break folks as part of these larger changes.
} | ||
beforeEach(async () => { | ||
await io.rmRF(rcFile); | ||
// if (fs.existsSync(rcFile)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code
version = semver.clean(version) || ''; | ||
let fileName: string = | ||
osPlat == 'win32' | ||
? `node-v${version}-win-${osArch}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If version
is an empty string, won't this file name be invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for all the review comments, makes it a lot easier to get context on the changes
@@ -1,31 +0,0 @@ | |||
// Jest Snapshot v1, https://goo.gl/fbAQLP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i never understood the snapshot thing. Nice to see it deleted :) one less thing to learn
|
||
console.log(`version: ${version}`); | ||
if (version) { | ||
let token = core.getInput('token'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider validating token is supplied
In checkout, I have seen customers with token: ''
i think it's related to the marketplace copy/paste
const registryUrl: string = core.getInput('registry-url'); | ||
const alwaysAuth: string = core.getInput('always-auth'); | ||
if (registryUrl) { | ||
auth.configAuthentication(registryUrl, alwaysAuth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be await auth.configAuthentication
? The implementation looks "sync" but unit tests await
src/installer.ts
Outdated
|
||
let downloadPath = ''; | ||
try { | ||
downloadPath = await tc.downloadTool(info.downloadUrl, undefined, token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt this use info.token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when downloading from nodejs.org, shouldnt use a token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or would it better to check the hostname of the download url to determine whether to pass token?
|
Bumps [husky](https://github.com/typicode/husky) from 5.0.9 to 5.1.0. - [Release notes](https://github.com/typicode/husky/releases) - [Commits](typicode/husky@v5.0.9...v5.1.0) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Node distributions are now available from https://github.com/actions/node-versions for reliability since the releases are backed by a CDN. That repo offers a versions-manifest.json which abstracts away the location of each version. Therefore in the future it could move from releases to some other CDN or endpoint. This set of changes queries the versions-manifest against the node version semver spec provided by the workflow to resolved to latest for that spec. If not found in the versions-manifest, it falls back to the node dist ( https://nodejs.org/dist/index.json )