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

support ls-remote command and git+ssh protocol #16

Merged
merged 3 commits into from
May 15, 2023

Conversation

jakecoffman
Copy link
Member

The integration test uses a Docker container that doesn't have ssh installed so the commands would fail normally. This is a simpler setup than building a Docker network and achieves the same result. If the docker build fails then the test fails!

@jakecoffman jakecoffman force-pushed the jakecoffman/support-ls-remote-and-git-ssh branch 2 times, most recently from db50c3e to 7a908bd Compare May 12, 2023 14:46
@jakecoffman jakecoffman force-pushed the jakecoffman/support-ls-remote-and-git-ssh branch from 7a908bd to 8842e5e Compare May 12, 2023 14:47
.github/workflows/go.yml Show resolved Hide resolved
@jakecoffman jakecoffman requested a review from a team as a code owner May 15, 2023 13:09
@jakecoffman jakecoffman merged commit ab75302 into main May 15, 2023
@jakecoffman jakecoffman deleted the jakecoffman/support-ls-remote-and-git-ssh branch May 15, 2023 13:16
@deivid-rodriguez
Copy link

deivid-rodriguez commented May 15, 2023

Did we try this against dependabot/dependabot-core#7258? I'm afraid it's now going to start "succeeding" now but generating incorrect PRs since it will use "private repo syntax" for the dependency using ssh.

@jakecoffman
Copy link
Member Author

@deivid-rodriguez I haven't cut a release of git-shim and bumped core yet, so all is good still. I'll repro it and see if this changes anything. Thanks for the heads up!

@deivid-rodriguez
Copy link

Ok, great! I mean it would not be the end of the world, but generally I consider a completely broken update better than a successful update with the wrong PR contents.

I did try changing git-shim last week to do this, but I think I didn't build the binary properly or I didn't put it at the right path, because it seemed to have no effect

I did change our insteadOf git config commands in core to re-write git+ssh urls too and that did cause the issue I'm taking about here, so I didn't proceed with making that change.

@jakecoffman
Copy link
Member Author

Just tried locally on main with the updated shim, it still fails, but on a git fetch

updater | Progress: resolved 1009, reused 1008, downloaded 0, added 0
updater |  ERROR  Command failed with exit code 128: git fetch --depth 1 origin 32e8e38c7fcd935241b9baab71bb432fd9b166ed
updater | error: cannot run ssh: No such file or directory
updater | fatal: unable to fork
updater | 
updater | pnpm: Command failed with exit code 128: git fetch --depth 1 origin 32e8e38c7fcd935241b9baab71bb432fd9b166ed
updater | error: cannot run ssh: No such file or directory
updater | fatal: unable to fork
updater |     at makeError (/home/dependabot/.cache/node/corepack/pnpm/8.3.1/dist/pnpm.cjs:24230:17)
updater |     at handlePromise (/home/dependabot/.cache/node/corepack/pnpm/8.3.1/dist/pnpm.cjs:24801:33)
updater |     at runMicrotasks (<anonymous>)
updater |     at processTicksAndRejections (node:internal/process/task_queues:96:5)
updater |     at async gitFetcher (/home/dependabot/.cache/node/corepack/pnpm/8.3.1/dist/pnpm.cjs:113224:11)
updater |     at async fetcher (/home/dependabot/.cache/node/corepack/pnpm/8.3.1/dist/pnpm.cjs:125583:16)
updater |     at async run (/home/dependabot/.cache/node/corepack/pnpm/8.3.1/dist/pnpm.cjs:124970:23)

No amount of argument rewriting can fix that since the URL isn't an argument. Looks like the upstream fix is definitely the way to go.

@deivid-rodriguez
Copy link

Well, that's good because this patch won't make the behaviour any worse, just make it fail a bit later. Thanks for verifying this!

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