-
Notifications
You must be signed in to change notification settings - Fork 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
Issues when updating git dependencies in PNPM #7258
Comments
Same situation here. Dependabot Update logs
Here is a live repo for repro. "difflib": "github:postlight/difflib.js", |
Thanks for that example, will make this much easier to fix confidently! |
@deivid-rodriguez We might be able to fix it with a small change to the Git shim. It's currently restricted to which commands to run here, and the protocols to rewrite here. |
Yep, I was just looking at that! |
We'd need to allow |
@deivid-rodriguez Yep, I think that would do it! |
Awesome, I'll look into it. This is reproducible with the CLI only, so I'll add a smoke tests since I'm at it! |
I wasn't lucky with the git-shim changes, but this other tweak seems to do the trick! diff --git a/common/lib/dependabot/shared_helpers.rb b/common/lib/dependabot/shared_helpers.rb
index 80294552d..9870c1ac9 100644
--- a/common/lib/dependabot/shared_helpers.rb
+++ b/common/lib/dependabot/shared_helpers.rb
@@ -274,6 +274,10 @@ module Dependabot
"git config --global --add url.https://#{host}/." \
"insteadOf git://#{host}/"
)
+ run_shell_command(
+ "git config --global --add url.https://#{host}/." \
+ "insteadOf git+ssh://git@#{host}/"
+ )
end
def self.reset_git_repo(path) |
@deivid-rodriguez probably still worth updating the pnpm smoke test to include this case? |
There's no pnpm smoke test yet 😳, I'll be adding one though to test this case, yeah. |
Just to post today's debugging of this issue, I found the culprit is here: In that method PNPM tries to do a manual HEAD request to Some debugging prints on the failing HEAD request shows this error:
It seems like some DNS issue/misconfiguration specific to our CLI/production environment 😬. One potential solution would be to change PNPM to use |
I think I figured it out! |
@deivid-rodriguez That makes sense. Since the Updater container is on a network where only HTTP(S) is allowed to the proxy, if it were to ignore the proxy setting the first thing it would do is make a DNS request, which will fail. When using the proxy the DNS request happens on the Proxy container. |
I opened an issue and a raw patch to PNPM! |
remove auto commit since it skips tests related: dependabot/dependabot-core#7258 dependabot/dependabot-core#7851 dependabot/dependabot-core#10124 pnpm/pnpm#6050 pnpm/pnpm#6530 pnpm/pnpm#8343
remove auto commit since it skips tests related: dependabot/dependabot-core#7258 dependabot/dependabot-core#7851 dependabot/dependabot-core#10124 pnpm/pnpm#6050 pnpm/pnpm#6530 pnpm/pnpm#8343
remove auto commit since it skips tests related: dependabot/dependabot-core#7258 dependabot/dependabot-core#7851 dependabot/dependabot-core#10124 pnpm/pnpm#6050 pnpm/pnpm#6530 pnpm/pnpm#8343
Is there an existing issue for this?
Package ecosystem
npm
Package manager version
PNPM
Language version
No response
Manifest location and content before the Dependabot update
I don't have a repro for this yet, but we're seeing issues in private repositories related to upgrading dependencies coming from git sources.
dependabot.yml content
No response
Updated dependency
No response
What you expected to see, versus what you actually saw
Updates should happen correctly without errors.
In particular, we see errors like the following:
I have not been able to reproduce this locally. Everytime I specify
git+ssh
on a package.json file,pnpm
ignores it and useshttps
. However, that's not happening in these repositories for some reason.Yarn & NPM have specific code to replace
git+ssh
withhttps
inpackage.json
before trying to update a lockfile, and then replacing back the updatedhttps
references withgit+ssh
again. So I guess this is a common problem in other package manager, so I will try to "blindly" fix it by applying the same fix.Some preparatory work for that is at #7245.
In general, we should also review how updates behave with
git
dependencies, since I have not done any explicit testing with that.Native package manager behavior
No response
Images of the diff or a link to the PR, issue, or logs
No response
Smallest manifest that reproduces the issue
No response
The text was updated successfully, but these errors were encountered: