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

fix Yarn 1 erroring on parent npmrc files with undefined vars #6883

Merged
merged 3 commits into from
Mar 24, 2023

Conversation

jakecoffman
Copy link
Member

If a repo has multiple .npmrc files, Dependabot only rewrites one of them to remove variables. NPM doesn't seem to have an issue with this, I assume it's just expanding undefined vars to "", but Yarn 1 is not ok with that and errors.

To fix this, I've added some code to rewrite all of the .npmrc files and remove the variables.

Still trying to figure out if there's a way to test this in this project, I have a CLI test that is passing with this change.

@jakecoffman jakecoffman requested a review from a team as a code owner March 21, 2023 17:54
@Nishnha
Copy link
Member

Nishnha commented Mar 21, 2023

Npm handles this a bit differently - instead of removing any variables, it only includes in the lines it needs into an .npmrc

def npmrc_content
initial_content =
if npmrc_file then complete_npmrc_from_credentials
elsif yarnrc_file then build_npmrc_from_yarnrc
else
build_npmrc_content_from_lockfile
end
return initial_content || "" unless registry_credentials.any?
([initial_content] + credential_lines_for_npmrc).compact.join("\n")
end

and then the content is written out into a temporary directory when performing updates

File.write(File.join(lockfile_directory, ".npmrc"), npmrc_content)

And it looks like Yarn v1 does the same thing

File.write(".npmrc", npmrc_content) unless Helpers.yarn_berry?(yarn_lock)

def build_npmrc_from_yarnrc
yarnrc_global_registry =
yarnrc_file.content.
lines.find { |line| line.match?(/^\s*registry\s/) }&.
match(NpmAndYarn::UpdateChecker::RegistryFinder::YARN_GLOBAL_REGISTRY_REGEX)&.
named_captures&.fetch("registry")
return "registry = #{yarnrc_global_registry}\n" if yarnrc_global_registry
build_npmrc_content_from_lockfile
end

You may need to update the way we add these these lockfiles to the temporary directory so that all of the .npmrc files are added and not just the 1

@jakecoffman
Copy link
Member Author

jakecoffman commented Mar 21, 2023

@Nishnha Yarn is using in_a_temporary_repo_directory which is the path the repo is cloned to, so there's no need to write more .npmrc files, they're already there! Which is actually why this problem exists. We need to clean those up too.

@jakecoffman
Copy link
Member Author

I created a Yarn 1 smoke test which also tests this issue since it required a clone to have certain files on disk, and I wasn't sure how I'd achieve that in the rspec tests: dependabot/smoke-tests#45

@jakecoffman jakecoffman merged commit 0bf0328 into main Mar 24, 2023
@jakecoffman jakecoffman deleted the jakecoffman/fix-yarn-multiple-npmrcs branch March 24, 2023 15:58
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.

3 participants