-
Notifications
You must be signed in to change notification settings - Fork 59
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
Generate RestRequest to pass to Rest Handlers #623
Conversation
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.
The change makes sense.
I wonder how much of these dependencies we will inherit from OpenSearch.
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.
Merging this in! We can continue the discussion in the comments/thread
#605 is still in progress and we can address any lingering questions there as well. |
And a reabse with
|
73a2d73
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #623 +/- ##
============================================
- Coverage 64.09% 63.36% -0.73%
- Complexity 252 253 +1
============================================
Files 50 50
Lines 1103 1122 +19
Branches 35 36 +1
============================================
+ Hits 707 711 +4
- Misses 385 400 +15
Partials 11 11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Turns out Fixed and rebased to main. |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-1.x 1.x
# Navigate to the new working tree
pushd ../.worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-623-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7b9ccde29e35d4858b290c828e059297610c8555
# Push it to GitHub
git push --set-upstream origin backport/backport-623-to-1.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-1.x Then, create a pull request where the |
@dbwiddis the backport failed for 1.x. Can we have a manual PR and clear the difference? |
I'll handle this |
Probably need backport #628 merged before trying to backport this. |
Should I replace |
* Generate RestRequest to pass to Rest Handlers * Update Response handling to use RestRequest (needs companion PR) Signed-off-by: Daniel Widdis <[email protected]> * Update Extension with new SDK signature (preview of AD extension PR) Signed-off-by: Daniel Widdis <[email protected]> * Fix tests Signed-off-by: Daniel Widdis <[email protected]> * Less diff on migration, yay! Signed-off-by: Daniel Widdis <[email protected]> * Remove stray reference to old type Signed-off-by: Daniel Widdis <[email protected]> * Typo fix Signed-off-by: Daniel Widdis <[email protected]> * CharSequence isEmpty() is a JDK 15+ feature Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]>
Hey @nassipkali sorry for any confusion. This comment on #605 is the latest status and I will post a complete answer to your question on that PR. |
* Generate RestRequest to pass to Rest Handlers * Update Response handling to use RestRequest (needs companion PR) * Update Extension with new SDK signature (preview of AD extension PR) * Fix tests * Less diff on migration, yay! * Remove stray reference to old type * Typo fix * CharSequence isEmpty() is a JDK 15+ feature --------- Signed-off-by: Daniel Widdis <[email protected]>
Companion PRs (this one must be merged after Core and before AD):
Description
Extensions do not use all the internal components of a
RestRequest
and initial implementation double-purposed theExtensionsRestRequest
object used in transport as the object that Extensions would need to handle. However, increasing use of the methods on this object are ending up duplicating a lot of code. It makes a lot more sense to just recreate aRestRequest
object from the parameters passed in this request.Issues Resolved
Part of SDK #431.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.