-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fixing Yarn1 erroring with failed to replace env #7767
Fixing Yarn1 erroring with failed to replace env #7767
Conversation
I have test it with Dependabot CLI and it passes and creates the PR successfully |
Create a smoke test for the same: dependabot/smoke-tests#113 |
@@ -380,9 +380,10 @@ def clean_npmrc_in_path(yarn_lock) | |||
dirs.pop | |||
while dirs.any? | |||
npmrc = dirs.join("/") + "/.npmrc" | |||
break unless File.exist?(npmrc) |
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 assume this was breaking and not walking through the entire dirs right?
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.
Yes. If the subdir did not contain the .npmrc
file then it was breaking out. In such case if the repo root dir still had .npmrc
file then error was getting raised.
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.
Looks great, nice catch!
Just had a question, purely out of curiosity!
npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/yarn_lockfile_updater.rb
Show resolved
Hide resolved
196e48d
to
a5f61a9
Compare
a5f61a9
to
642c17b
Compare
Context
If a repo has multiple .npmrc files, Dependabot only rewrites one of them to remove variables. NPM doesn't have an issue but Yarn 1 is not ok and errors out.
This issue was fixed in the PR but one of the edge case was to leading to raise the issue again.
Edge case:
User root dir contained the
.npmrc
file with it's sub-dir (foo/bar/foo1/bar1/yarn.lock). The Dependabot npm job was failing for path/foo/bar/foo1/bar1
with the below error:Approach
The modified code now checks all the parent directories for
.npmrc files
, whereas the previous code used to stopped, if a directory did not have one. Now I have removed the break statement and continually performing dirs.pop in each iteration, the loop in the modified code advances up to the root directory. This modification make sures that all relevant.npmrc files
are addressed. Also a minor regex change was done in the gsub technique for non-greedy matching.