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

feat: add curl command in yaml #929

Conversation

AkashKumar7902
Copy link
Member

Related Issue

  • Info about Issue or bug

Closes: #881

Describe the changes you've made

I have added a new entry in HttpReq struct named Curl in which string representation of curl command will be stored. I have defined a function named makeCurlCommand to get the string representation of the curl command

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, local variables)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Please let us know if any test cases are added

Please describe the tests(if any). Provide instructions how its affecting the coverage.

Describe if there is any unusual behaviour of your code(Write NA if there isn't)

N/A

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Screenshots (if any)

Original Updated
image image

Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Copy link
Member

@charankamarapu charankamarapu left a comment

Choose a reason for hiding this comment

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

Please resolve these comments

pkg/util.go Outdated
curl := fmt.Sprintf("curl --request %s \\\n", method)
curl = curl + fmt.Sprintf(" --url %s \\\n", url)
for k, v := range header {
if k == "Content-Length" {
Copy link
Member

Choose a reason for hiding this comment

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

Have you debugged why providing content-length in curl is throwing an error when you use the curl..?

Copy link
Member Author

Choose a reason for hiding this comment

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

@charankamarapu I found the issue. It is happening because of the extra spaces before "url" field of --data.

with extra space without extra space
image image

Copy link
Member

Choose a reason for hiding this comment

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

Ok then try to find out why there is an extra space and remove it. Add content-length header also in the the curl.

Copy link
Member Author

Choose a reason for hiding this comment

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

the extra spaces is because curl command is part of req in .yaml. The only solution is to make curl command a top level field in .yaml.
when selecting curl command, you can see that there will also be extra spaces :
image

Copy link
Member

@charankamarapu charankamarapu Oct 1, 2023

Choose a reason for hiding this comment

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

Oh I see, I guess keeping curl in top level field looks fine for me. Because curl section shouldn't be under spec, as spec majorly tells how keploy decoded its req and resp. Put the curl section before spec or may be at the last. Btw great work in analysing the bug.

@@ -132,3 +132,16 @@ func GenerateRandomID() int {
id := rand.Intn(1000000000) // Adjust the range as needed
return id
}

func MakeCurlCommand(method string, url string, header map[string]string, body string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Add checks for param and type of methods . Like if there is a body then only you need to add --body etc . These kind of checks.

Copy link
Member Author

@AkashKumar7902 AkashKumar7902 Oct 1, 2023

Choose a reason for hiding this comment

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

@charankamarapu I have added a body check in makeCurlCommand like this :
image
any invalid method type would result in 400 error and it won't make a .yaml file and also makeCurlCommand function won't be called

Copy link
Member

Choose a reason for hiding this comment

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

Ok got it.

@@ -132,3 +132,16 @@ func GenerateRandomID() int {
id := rand.Intn(1000000000) // Adjust the range as needed
return id
}

func MakeCurlCommand(method string, url string, header map[string]string, body string) string {
curl := fmt.Sprintf("curl --request %s \\\n", method)
Copy link
Member

Choose a reason for hiding this comment

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

Add error handling.

Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902
Copy link
Member Author

AkashKumar7902 commented Oct 1, 2023

@charankamarapu At any level at least 4 spaces will always be present:
image
Should i remove the content-length as i did earlier ?

@charankamarapu
Copy link
Member

@charankamarapu At any level at least 4 spaces will always be present: image Should i remove the content-length as i did earlier ?

I guess there is no other way . Yes do it as eariler.

Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@charankamarapu
Copy link
Member

Hi @AkashKumar7902 are all changes done can I review the PR..?

@AkashKumar7902
Copy link
Member Author

@charankamarapu yeah

Copy link
Member

@charankamarapu charankamarapu left a comment

Choose a reason for hiding this comment

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

Ship it!

@charankamarapu charankamarapu merged commit 7947b9a into keploy:main Oct 3, 2023
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature]: Add curl section in the testcase yamls
2 participants