-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Conversation
Pinging @elastic/es-core-infra |
90ffa39
to
6795b87
Compare
* 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. | ||
* |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Julie
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:VersionSensitiveWarningsHandler
to check for specific warning messages.Addresses #36468.