-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Backport 2.18] Downgrade version to 2.18.0 for ser/de of new parameters of RequestRequest #16477
Conversation
Signed-off-by: Spencer G. Jones <[email protected]> (cherry picked from commit 936cdb9) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.
LTGM. In 2.18.0 these are precisely the same value but if we need to bump the 2.18 branch to version 2.18.1 this would be needed for BWC.
It's OK to wait until 2.18.1 version bump on this branch to merge, though.
❌ Gradle check result for 6c8e83d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.18 #16477 +/- ##
=========================================
Coverage 71.83% 71.83%
+ Complexity 65381 65343 -38
=========================================
Files 5312 5312
Lines 304995 304995
Branches 44440 44440
=========================================
+ Hits 219090 219104 +14
+ Misses 67614 67608 -6
+ Partials 18291 18283 -8 ☔ View full report in Codecov by Sentry. |
Reminder: merge this when the below PR is merged: |
2.18.0 has already sailed, does this bug require patch release? |
Nope, this only exists for BWC in the case a user might have to do a rolling upgrade from 2.18.0 to 2.18.1 (if it ever gets released). Which is why we're waiting for the 2.18.1 version bump to merge it. Also, it's not a bug, it's an apparently common pattern of adding a new feature with version checks using "CURRENT" and waiting for the backport (since it wouldn't compile otherwise) and then once it's available in the backport changing to actual version numbers. There should be a better way to do that, but it is what it is. |
Given the headaches with the other Current -> 2.18 PR, going ahead and merging this now, before the version bump (thinking it could possibly break a BWC test). |
Backport 936cdb9 from #16472.