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

Make sure that BWC tests run successfully, even with types deprecation messages. #36511

Merged
merged 3 commits into from
Dec 12, 2018

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Dec 11, 2018

When deprecating types in get requests in #35930, I updated several REST tests to use the type name _doc to avoid deprecation warnings. However, some of these tests are run as part of our BWC tests, and can incorporate nodes from early 6.x versions where types were not allowed to begin with underscores. To fix the BWC tests, this PR makes the following changes:

  • Revert the change to type name in BWC tests, so that early 6.x nodes can accept requests without errors.
  • Update get requests to allow for types deprecation warnings in 7.0. In doing so, I've switched to the recently-introduced VersionSensitiveWarningsHandler to check for specific warning messages.

Addresses #36468.

@jtibshirani jtibshirani added >test Issues or PRs that are addressing/adding tests :Core/Infra/REST API REST infrastructure and utilities v7.0.0 labels Dec 11, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jtibshirani jtibshirani changed the title Make sure that the BWC can run successfully, even with types deprecation messages. Make sure that BWC tests run successfully, even with types deprecation messages. Dec 11, 2018
* Creates request options designed to be used when calling a deprecated 'typed' API. The
* options will ensure that the given warnings are returned if all nodes are on 7.x, and will
* allow (but not require) the warnings if any node is running 6.x.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change the javadoc to talk about Version.CURRENT otherwise the comment won't age well when we move to 8 and beyond

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

* @param warnings The expected types removal warnings.
* @return A {@link RequestOptions} instance for use with a deprecated 'typed' request.
*/
public static RequestOptions expectTypesWarnings(String... warnings) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function looks to be general and not specifically for types. I wonder if it is worth calling it something like requireWarningsNodesSameVersion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically speaking it would be requireWarningsIfNodesSameVersionAndAllowWarningsIfMixedVersions
That's obviously a bit of a mouthful so maybe just expectWarnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll update to expectWarnings and generalize the javadoc.

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

thanks Julie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities >test Issues or PRs that are addressing/adding tests v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants