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 bad_request to the rest-api-spec catch params #26539

Merged
merged 10 commits into from
Sep 14, 2017

Conversation

hub-cap
Copy link
Contributor

@hub-cap hub-cap commented Sep 7, 2017

This adds another request to the catch params. It also makes sure that
the generic request param does not allow 400 either.

This adds another request to the catch params. It also makes sure that
the generic request param does not allow 400 either.
@hub-cap
Copy link
Contributor Author

hub-cap commented Sep 7, 2017

Please note I did not change the lower bounds on the generic request param check. I thought it more concrete to say "anything 400 and above except for this list" rather than "anything above 401 but not these" since we now check for 400 and 401.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM assuming that @elastic/es-clients is good with the addition.

@jasontedor
Copy link
Member

Yes, I prefer the way that you've done it here.

@hub-cap
Copy link
Contributor Author

hub-cap commented Sep 7, 2017

@elasticmachine test this please

for some reason I dont see the integration for CI on this PR anymore?

image

@polyfractal
Copy link
Contributor

You might consider adding a "feature skip" to these tests, otherwise every client is going to get whacked with 20+ failures until client authors can update their test runner.

E.g.

 - skip:
      features: bad_request

I guess it depends how urgently you want client authors to update. Skip == when they get a chance, no skip == lotsa failures == asap.

@hub-cap
Copy link
Contributor Author

hub-cap commented Sep 7, 2017

oh my. Thats not good. Let me sync w/ the client team and see how they would like to proceed. Id love to not break builds for them. Ty for the heads up @polyfractal

@polyfractal
Copy link
Contributor

OTOH, I think most of the client team uses breakages as a signal that something needs updating. It's easier than trying to closely track upstream changes (and we're lazy) :)

I'm ++ on the bad_request addition btw, but would be good to run it by the rest of the client team.

@Mpdreamz
Copy link
Member

@hub-cap informed the #lang-clients room and @polyfractal raised the possible breakage during the last clients meeting which did not seem to rattle anyone 😄. I feel enough time has passed for anyone from @elastic/es-clients to object to the breakage.

@hub-cap
Copy link
Contributor Author

hub-cap commented Sep 14, 2017

@elasticmachine test this please

@hub-cap hub-cap merged commit f385e0c into elastic:master Sep 14, 2017
hub-cap added a commit that referenced this pull request Sep 15, 2017
This adds another request to the catch params. It also makes sure that
the generic request param does not allow 400 either.
hub-cap added a commit that referenced this pull request Sep 15, 2017
This adds another request to the catch params. It also makes sure that
the generic request param does not allow 400 either.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 16, 2017
* master:
  fix testSniffNodes to use the new error message
  Add check for invalid index in WildcardExpressionResolver (elastic#26409)
  Docs: Use single-node discovery.type for dev example
  Filter unsupported relation for range query builder (elastic#26620)
  Fix kuromoji default stoptags (elastic#26600)
  [Docs] Add description for missing fields in Reindex/Update/Delete By Query (elastic#26618)
  [Docs] Update ingest.asciidoc (elastic#26599)
  Better message text for ResponseException
  [DOCS] Remove edit link from ML node
  enable bwc testing
  fix StartRecoveryRequestTests.testSerialization
  Add bad_request to the rest-api-spec catch params (elastic#26539)
  Introduce a History UUID as a requirement for ops based recovery  (elastic#26577)
  Add missing catch arguments to the rest api spec (elastic#26536)
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.

4 participants