Skip to content
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

Checks if the new corepack home folder exists and creates it if necessary (Fix for issue #198) #200

Merged
merged 11 commits into from
Oct 28, 2022

Conversation

Gamadril
Copy link
Contributor

No description provided.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test and update the PR title and description to describe what is going on? Seeing Fix for issue #198 in the commit history doesn't really say much about what happened.

@Gamadril Gamadril changed the title Fix for issue #198 Checks if the new corepack home folder exists and creates it if necessary (Fix for issue #198) Oct 20, 2022
@aduh95
Copy link
Contributor

aduh95 commented Oct 20, 2022

Thanks for sending that. However I don't think that's where you want to put that command, instead if should be before the tar.c invocation:

await tar.c({gzip: true, cwd: baseInstallFolder, file: path.resolve(outputPath)}, installLocations.map(location => {
return path.relative(baseInstallFolder, location);
}));

@Gamadril
Copy link
Contributor Author

Prepare is not Hydrate... where actually the error happens...

@aduh95
Copy link
Contributor

aduh95 commented Oct 20, 2022

Prepare is not Hydrate... where actually the error happens...

Yeah we would need one before tar.x as well:

await tar.x({file: fileName, cwd: installFolder}, [`${name}/${reference}`]);

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mkdir call needs to happen just before tar.c in Prepare.ts and tar.x in Hydrate.ts.

sources/folderUtils.ts Outdated Show resolved Hide resolved
sources/commands/Hydrate.ts Outdated Show resolved Hide resolved
sources/commands/Hydrate.ts Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Oct 26, 2022

@Gamadril Any chance you'll have time to address the remaining suggestions and add some tests so we can land this?

@Gamadril
Copy link
Contributor Author

hmm, seems to be more complicated than expected. will have to investigate more time...

sources/commands/Hydrate.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, would you like to add a fix and a test for the prepare command as well?

tests/main.test.ts Outdated Show resolved Hide resolved
@aduh95 aduh95 merged commit 7b5f2f9 into nodejs:main Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants