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

Add enableUnixSockets option #2062

Merged
merged 20 commits into from Jul 21, 2022
Merged

Add enableUnixSockets option #2062

merged 20 commits into from Jul 21, 2022

Conversation

TommyDew42
Copy link
Contributor

@TommyDew42 TommyDew42 commented Jun 18, 2022

What is the PR for?

For issue #2046, we add an option to control if unix domain socket is supported:

// enableUnixSocket is default to false before Got v13
const gotUnixSocketEnabled = got.extend({enableUnixSocket: true});

How is the PR tested?

Unit tests.

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates in both the README and the types.

@TommyDew42 TommyDew42 marked this pull request as ready for review June 18, 2022 13:52
@sindresorhus
Copy link
Owner

Thanks for working on this. You also need to update the documentation.

https://github.com/sindresorhus/got/blob/main/documentation/tips.md#unix-domain-sockets can be moved to the options docs for this option.

documentation/2-options.md Outdated Show resolved Hide resolved
documentation/2-options.md Outdated Show resolved Hide resolved
documentation/2-options.md Outdated Show resolved Hide resolved
await got('http://unix:/var/run/docker.sock:/containers/json');

// Or without protocol (HTTP by default)
await got('unix:/var/run/docker.sock:/containers/json');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the actual option.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we actually include the option in all examples? This way we'd encourage users to explicitly enable/disable UNIX sockets, which will ease transitioning to a new major release.

documentation/tips.md Outdated Show resolved Hide resolved
documentation/tips.md Show resolved Hide resolved
documentation/2-options.md Show resolved Hide resolved
test/redirects.ts Outdated Show resolved Hide resolved
@TommyDew42
Copy link
Contributor Author

@sindresorhus Do you think rerunning the test will work? Don’t think the extra if-statement made us exceed the limit.

documentation/2-options.md Outdated Show resolved Hide resolved
documentation/2-options.md Outdated Show resolved Hide resolved
documentation/2-options.md Outdated Show resolved Hide resolved
documentation/2-options.md Outdated Show resolved Hide resolved
test/redirects.ts Outdated Show resolved Hide resolved
test/unix-socket.ts Outdated Show resolved Hide resolved
test/unix-socket.ts Outdated Show resolved Hide resolved
test/unix-socket.ts Outdated Show resolved Hide resolved
documentation/2-options.md Outdated Show resolved Hide resolved
test/unix-socket.ts Outdated Show resolved Hide resolved
documentation/2-options.md Outdated Show resolved Hide resolved
source/core/options.ts Outdated Show resolved Hide resolved
source/core/options.ts Outdated Show resolved Hide resolved
source/core/options.ts Show resolved Hide resolved
test/unix-socket.ts Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Unix domain socket option Add enableUnixSockets option Jul 20, 2022
Copy link
Collaborator

@szmarczak szmarczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TommyDew42
Copy link
Contributor Author

TommyDew42 commented Jul 21, 2022

Seems the release of node v18.6.0 on 13/7 broke the tests. Is this commit that failed the test?

I run tests locally with node v18.5.0 and all tests are passed without errors.

@szmarczak
Copy link
Collaborator

szmarczak commented Jul 21, 2022

Don't worry about that. We'll fix that in the main branch, that doesn't stop us from merging :)

const gotUnixSocketsDisabled = got.extend({enableUnixSockets: false});

t.false(gotUnixSocketsDisabled.defaults.options.enableUnixSockets);
Copy link
Collaborator

@szmarczak szmarczak Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite useless. You can just call got('unix:', {enableUnixSockets: false}) which does the same.

@szmarczak szmarczak merged commit 461b3d4 into sindresorhus:main Jul 21, 2022
6 of 9 checks passed
@szmarczak
Copy link
Collaborator

Thanks @TommyDew42 ❤️

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.

None yet

3 participants