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 new request from cURL command #840

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

snippetkid
Copy link
Contributor

@snippetkid snippetkid commented Oct 31, 2023

Description

This PR add the capability to create an HTTP request from a valid cURL command (fixes #338)

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Changes:

  • Added From Curl option in the New Request modal.
  • Added parse-curl as a dependency and new utility method getRequestFromCurlCommand to parse cURL commands

@snippetkid snippetkid changed the title feat: add new request from curl feature Add new request from cURL command Oct 31, 2023
@snippetkid
Copy link
Contributor Author

cURL2Request.mp4

@snippetkid snippetkid marked this pull request as ready for review October 31, 2023 20:17
@Olian04
Copy link

Olian04 commented Nov 1, 2023

@snippetkid how does this handle the  -k, --insecure flag? Seeing as bruno currently doesn't support disabling TLS/SSL certificate verification on an individual request basis.

Looking into the parse-curl package, it seems like that package only supports a small subset of curls flags.

Specifically -A, --user-agent, -H, --header, -d, --data, --data-ascii, -u, --user, -I, --head, -X, --request, -b, --cookie, and --compressed.
I suggest we postpone implementing this feature until it can be implemented properly.

@snippetkid
Copy link
Contributor Author

@Olian04 I think your question answers itself. Since bruno doesn't support insecure requests, so does the PR. Neither do I see -k, --insecure in Insomnia's supported args (which you can take a look here) so my question would be do we really need that?

Apologies but I fail to understand what is improper in this PR. I believe this PR might be a valuable addition for common use cases, even in its current state. What I suggest is an incremental approach. We can proceed with merging this PR to offer this feature to users. This way, we can provide immediate value to users while working towards a more comprehensive feature.

I am keen to hear your thoughts on this @helloanoop

@Olian04
Copy link

Olian04 commented Nov 1, 2023

@snippetkid Bruno does support insecure requests. However it only supports it as a global option, not on a request by request basis. I bring up this flag in particular since my current use case for bruno requires some requests to be made insecurely. However I'm sorry but I fail to see how quoting another inadequate implementation (Insomnia) is a good argument for our implementation to also be inadequate.

I would accept this PR as an incremental implementation if it parsed all flags but warned the user when an unsupported flag was detected.

@helloanoop helloanoop merged commit 0e0f04c into usebruno:main Nov 1, 2023
2 checks passed
@helloanoop
Copy link
Contributor

@Olian04 A lot of users are waiting for curl support #338
We will consider this as an incremental implementation.

I bring up this flag in particular since my current use case for bruno requires some requests to be made insecurely. However it only supports it as a global option, not on a request by request basis.

Can you raise an issue for this. I am not sure how other apps do this at a request level (via UI). We should be able to clearly support an api that can turn of ssl verification at the request level req.disableSslVerification()

@helloanoop
Copy link
Contributor

Nice work @snippetkid !

Merged !! This will go out in the upcoming 1.1.0 release

@snippetkid
Copy link
Contributor Author

Thanks for the merge @helloanoop 🚀 I'll keenly follow this feature and related issues and come up with ways to improve the curl parsing. Insomnia, Hoppscotch and RecipeUI have their own curl parsing, so I guess we should write our own parser and replace the parse-curl package in the future if it has shortcomings.

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.

Convert curl commands to request.
3 participants