Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

validate all 2xx status code in Webhook response #49 #50

Merged

Conversation

mathewlsm
Copy link
Contributor

Reststatus.Created for WebHook-Response is also acceptable (#49).

Issue #49:

Description of changes: Reststatus 201 is also acceptable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Reststatus.Created for WebHook-Response is also acceptable (opendistro-for-elasticsearch#49).
@jinsoor-amzn
Copy link
Contributor

Hi,

Thank you for the PR. Can you please change so that it allows all of 2xx HTTP status code?

@jinsoor-amzn jinsoor-amzn self-requested a review May 8, 2019 21:48
using all 2XX response status as valid response
@mathewlsm
Copy link
Contributor Author

Hi,

I just added all 2XX status codes as valid response.

@lucaswin-amzn lucaswin-amzn self-requested a review May 9, 2019 15:07
Copy link
Contributor

@lucaswin-amzn lucaswin-amzn 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 to me, thanks for making this change!

@mihirsoni mihirsoni changed the title Improvement for Issue #49 validate all 2xx status code in Webhook response #49 May 9, 2019
Copy link
Contributor

@mihirsoni mihirsoni left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for the changes.

@lucaswin-amzn lucaswin-amzn merged commit 350515b into opendistro-for-elasticsearch:master May 9, 2019
/**
* all valid response status
*/
private static final Set<Integer> VALID_RESPONSE_STATUS = new HashSet<>(
Copy link
Member

Choose a reason for hiding this comment

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

Np: Should we call SUCCESS_RESPONSE_STATUS? To me all the http codes are valid(1XX, 3XX, 4XX)

tlfeng pushed a commit that referenced this pull request Feb 6, 2021
* Issue #49

using all 2XX response status as valid response, fixes #50 & fixes #49
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.

5 participants