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 --unix-socket option #2288

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

ztittle
Copy link
Contributor

@ztittle ztittle commented Jan 5, 2024

This is a feature provided by cURL that allows sending requests to a web server via a Unix Domain Socket. Some use cases include:

  • Querying docker API
  • Running a web server w/o opening a TCP port for security and/or performance reasons

https://curl.se/docs/manpage.html#--unix-socket

@ztittle ztittle marked this pull request as ready for review January 5, 2024 00:25
@jcamiel
Copy link
Collaborator

jcamiel commented Jan 5, 2024

Hi @ztittle thank you for this huge PR, this might have been quite difficult to see all the "small" things to cover everything!
I've made some comments on the PR to help to make all the CI checks green:

  1. clippy stuff should be fixed with my comments (you can test it locally by running cargo clippy)
  2. Black stuff is the linting on Python files. You can run black in your workspace and that should be OK
  3. Coverage stuff will be fixed with my comments (you should make a pattern file to avoid libcurl message, see comments)
  4. Crates stuff should be fixed if you rebase your PR

Don't hesitate to ask questions, even basic ones, if you need any help.

As you've done a tremendous work as a first contributor with this PR, don't hesitate also to give us feedbacks on what we can improve to make people read/understand the code. This can be anything from better documentation (like README.md on CONTRIBUTING.md), how to run tests, code sample, code comments etc...

integration/README.md Show resolved Hide resolved
integration/hurl/tests_failed/unix_socket.err Outdated Show resolved Hide resolved
integration/hurl/tests_ok/unix_socket.ps1 Outdated Show resolved Hide resolved
packages/hurl/src/http/client.rs Outdated Show resolved Hide resolved
@jcamiel jcamiel linked an issue Jan 5, 2024 that may be closed by this pull request
@ztittle ztittle force-pushed the add-unix-socket-option branch from 0c13df1 to e57b96e Compare January 6, 2024 03:54
@ztittle
Copy link
Contributor Author

ztittle commented Jan 6, 2024

Hi @ztittle thank you for this huge PR, this might have been quite difficult to see all the "small" things to cover everything! I've made some comments on the PR to help to make all the CI checks green:

  1. clippy stuff should be fixed with my comments (you can test it locally by running cargo clippy)
  2. Black stuff is the linting on Python files. You can run black in your workspace and that should be OK
  3. Coverage stuff will be fixed with my comments (you should make a pattern file to avoid libcurl message, see comments)
  4. Crates stuff should be fixed if you rebase your PR

Don't hesitate to ask questions, even basic ones, if you need any help.

As you've done a tremendous work as a first contributor with this PR, don't hesitate also to give us feedbacks on what we can improve to make people read/understand the code. This can be anything from better documentation (like README.md on CONTRIBUTING.md), how to run tests, code sample, code comments etc...

Thanks for the quick and very useful feedback!

One thing that took me a little bit of time to grok was understanding how the integration tests were setup, such as the python flask modules. But overall, it wasn't too bad and I liked how it's structured. Something that could be useful for future contributors would be a single script that can be ran that would automate code cleanup/linting, starting the test server, and running the integration tests (with the expected console column width :)) before a pr is created.

@jcamiel
Copy link
Collaborator

jcamiel commented Jan 6, 2024

Hi, thanks for the feedbacks, appreciated.

The least we can do is to improve the docs, I've noted this.

There are still two failed checks:

  1. first one is simple. We've reorganized the integration tests so it's not explicit yet. We insure to have 100% coverage for hurlfmt. You just have to take a Hurl file with an [Options] section and with a unix-socket option, export it to HTML and JSON with hurlfmt, and commit it to integration/hurlfmt/tests_ok. That should be OK for the coverage

  2. Second one is one windows. In the logs hurl --version shows that libcurl support unix socket so it's not about not supporting unix socket (we could revert the ps1 script btw). Maybe the Unix socket path should use \ instead of / on Windows? I will check the next week with @lepapareil if we can see why it's failing on Windows

@ztittle
Copy link
Contributor Author

ztittle commented Jan 6, 2024

Hi, thanks for the feedbacks, appreciated.

The least we can do is to improve the docs, I've noted this.

There are still two failed checks:

  1. first one is simple. We've reorganized the integration tests so it's not explicit yet. We insure to have 100% coverage for hurlfmt. You just have to take a Hurl file with an [Options] section and with a unix-socket option, export it to HTML and JSON with hurlfmt, and commit it to integration/hurlfmt/tests_ok. That should be OK for the coverage
  2. Second one is one windows. In the logs hurl --version shows that libcurl support unix socket so it's not about not supporting unix socket (we could revert the ps1 script btw). Maybe the Unix socket path should use \ instead of / on Windows? I will check the next week with @lepapareil if we can see why it's failing on Windows
  1. Makes sense. Will do.
  2. I looked into the failing windows test a bit more and it's not going to work with the python test server. There is an open issue (Enable AF_UNIX support in Windows python/cpython#77589) about supporting AF_UNIX for Windows, but it is blocked on a technicality on how Unix Domain Sockets is implemented in Windows (missing DGRAM support). It might be possible to create a unix domain socket in windows in rust (I found this crate https://crates.io/crates/uds_windows) for testing purposes, but it might be best to remove the windows integration tests for now.

@jcamiel
Copy link
Collaborator

jcamiel commented Jan 6, 2024

Yep, we can do this for the moment: in the ps1 file returns 255 unconditionally with your comment about why we're skipping it for the moment should be perfect

@ztittle ztittle force-pushed the add-unix-socket-option branch 2 times, most recently from b60c435 to 2b7e3a7 Compare January 6, 2024 18:43
This is a feature provided by cURL that allows sending requests to
a web server via a Unix Domain Socket. Some use cases
include:

* Querying docker API
* Running a web server w/o opening a TCP port for security and/or performance reasons

https://curl.se/docs/manpage.html#--unix-socket
@ztittle ztittle force-pushed the add-unix-socket-option branch from 2b7e3a7 to 98b0f50 Compare January 6, 2024 19:06
@jcamiel
Copy link
Collaborator

jcamiel commented Jan 6, 2024

/accept

@hurl-bot
Copy link
Collaborator

hurl-bot commented Jan 6, 2024

🕗 /accept is running, please wait for completion.

@hurl-bot
Copy link
Collaborator

hurl-bot commented Jan 6, 2024

✅ Pull request merged and closed by jcamiel with fast forward merge..

# List of commits merged from ztittle/hurl/add-unix-socket-option branch into Orange-OpenSource/hurl/master branch:

@hurl-bot hurl-bot merged commit 98b0f50 into Orange-OpenSource:master Jan 6, 2024
18 checks passed
@jcamiel
Copy link
Collaborator

jcamiel commented Jan 6, 2024

PR is now merged, thanks for your work @ztittle!

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.

Add --unix-socket option
3 participants