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: smarter git ssh override #194

Merged
merged 5 commits into from
Jul 9, 2024
Merged

Conversation

dennishenry
Copy link
Contributor

Removing the overwriting of git ssh commands. This should not be done, and should be left to individuals using their own ssh configs. This breaks multiple usecases.

Removing git ssh command that overrides local git ssh commands.

References

npm/cli#2891
#193
#129

@dennishenry
Copy link
Contributor Author

CC @wraithgar possibly you can take a look as this issue is plauging many in the git/node community

package.json Outdated Show resolved Hide resolved
lib/opts.js Outdated Show resolved Hide resolved
lib/opts.js Outdated Show resolved Hide resolved
test/opts.js Outdated Show resolved Hide resolved
lib/opts.js Outdated Show resolved Hide resolved
lib/opts.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

Having the tests look at the actual os.homedir() git config makes me nervous. I don't think that's a very safe thing to be doing. We may want to combine tap.mock and tap.testdir to manually generate the files we want in tests and have os.homedir() return the testdir instead.

The code itself is looking good except for coverage, which my suggestion should hopefully fix.

@dennishenry
Copy link
Contributor Author

@wraithgar I dropped using os.homedir() in the tests and applied all other suggestions. Let me know what you think now!

lib/opts.js Outdated Show resolved Hide resolved
lib/opts.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

Thanks for sticking with this @dennishenry. Testing this is not easy. I think we're very very close here.

@dennishenry
Copy link
Contributor Author

@wraithgar Should be good to go now! Let me know what you think and thanks so much for the partnership!

@dennishenry
Copy link
Contributor Author

@wraithgar Anything else you needed from me here? Just wanted to follow up. Thanks!

@wraithgar
Copy link
Member

Not at this time. Thanks for your patience here, other tasks have taken focus this month.

@wraithgar wraithgar changed the title fix: remove git ssh override fix: smarter git ssh override Jul 9, 2024
@wraithgar wraithgar merged commit 2135513 into npm:main Jul 9, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Jul 9, 2024
wraithgar pushed a commit that referenced this pull request Jul 9, 2024
🤖 I have created a release *beep* *boop*
---


## [5.0.8](v5.0.7...v5.0.8)
(2024-07-09)

### Bug Fixes

*
[`2135513`](2135513)
[#194](#194) smarter git ssh override
(#194) (@dennishenry, pacotedev)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

2 participants