Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

[KOGITO-1729] Consolidate HTTP Request parameters into one struct type #361

Merged
merged 1 commit into from
Jun 1, 2020

Conversation

MarianMacik
Copy link
Member

https://issues.redhat.com/browse/KOGITO-1729

JIRA was created as a result of the feedback in #257 (comment).

Things to discuss:

  1. Name of the struct is HTTPRequestInfo.
    I didn't want to name it HTTPRequest as then the parameter name, e.g. request, would clash with the actual request from Go's http package and it would be confusing in methods like for example here:
    https://github.com/kiegroup/kogito-cloud-operator/blob/e9069757dcc2fa0068019cc3a76d49a90719f139/test/framework/http.go#L53-L56 I was also considering HTTPRequestData but this isn't just data and HTTPRequestMetaData but that's too long. The parameter would then need to be something like requestMetaData etc.

  2. I think the goal of the JIRA was achieved, HTTP methods have much shorter signatures now, but now we need to call the constructor to create an HTTPRequestInfo struct each time we want to issue a request. Also some logging methods are now longer as struct fields needs to be accessed using the prefix.

  3. I have also found a small bug, so I decided to fix that:
    https://github.com/kiegroup/kogito-cloud-operator/blob/e9069757dcc2fa0068019cc3a76d49a90719f139/test/steps/http.go#L91-L92

Many thanks for submiting your Pull Request ❤️ 🏎️!

Please make sure that your PR meets the following requirements:

  • You have read the contributors guide
  • Pull Request title is properly formatted: [KOGITO-XYZ] Subject
  • Pull Request contains link to the JIRA issue
  • Pull Request contains description of the issue
  • Pull Request does not include fixes for issues other than the main ticket
  • Your feature/bug fix has a unit test that verifies it
  • You've tested the new feature/bug fix in an actual OpenShift cluster

Copy link
Contributor

@radtriste radtriste left a comment

Choose a reason for hiding this comment

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

lgtm

if err != nil {
return false, err
}

return strings.Contains(resultBody, responseContent), nil
}

// NewHTTPRequestInfo constructor creates a new HTTPRequestInfo struct
func NewHTTPRequestInfo(httpMethod, uri, path, bodyFormat, bodyContent string) HTTPRequestInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about providing separate function for GET and POST types?
For GET type you don't need to supply bodyFormat and bodyContent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I was actually thinking about it, but just to get rid of GET and POST strings. But your suggestion is even better, thanks!

Copy link
Contributor

@sutaakar sutaakar left a comment

Choose a reason for hiding this comment

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

Looks good, one suggestion in comment.

@MarianMacik
Copy link
Member Author

Added 2 methods:

  • NewGETHTTPRequestInfo
  • NewPOSTHTTPRequestInfo

I tried to change it to NewHTTPRequestInfoGET, but that would change the meaning slightly. Also I think GET should be all caps, so inevitably we will have a lot of capital letters together. If you know a better approach, feel free to suggest it.

@MarianMacik MarianMacik marked this pull request as ready for review May 28, 2020 16:42
@sutaakar
Copy link
Contributor

Looks good to me.

@spolti
Copy link
Member

spolti commented Jun 1, 2020

Folks, this PR was not merged yet because the CI is reporting error?

@sutaakar
Copy link
Contributor

sutaakar commented Jun 1, 2020

I guess so, seems to be approved and passing checks.

@MarianMacik
Copy link
Member Author

Hi, on Friday I was unable to restart the build, but @radtriste did and not it is finally green! Merging. Thanks all for review.

@MarianMacik MarianMacik merged commit 982153d into apache:master Jun 1, 2020
@ricardozanini ricardozanini added this to the v0.11.0 milestone Jun 2, 2020
@ricardozanini ricardozanini added bdd-tests 🧪 PR is related to the BDD test framework ready 🚀 PR is ready to be merged labels Jun 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bdd-tests 🧪 PR is related to the BDD test framework ready 🚀 PR is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants