Skip to content

[Backport-1.x]Avoid override of routes() in BaseRestHandler to respect the default behavior defined in RestHandler (#889) #991

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 22, 2021
Merged

[Backport-1.x]Avoid override of routes() in BaseRestHandler to respect the default behavior defined in RestHandler (#889) #991

merged 1 commit into from
Jul 22, 2021

Conversation

cliu123
Copy link
Member

@cliu123 cliu123 commented Jul 21, 2021

Description

[Describe what this change achieves]

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

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.

@cliu123 cliu123 changed the title [Backport-1.x]Avoid override of routes() in BaseRestHandler to respect the default behavior defined in RestHandler (#889) [Backport-1.x]Avoid override of routes() in BaseRestHandler to respect the default behavior defined in RestHandler (#889) & Introduce replaceRoutes() method and 2 new constructors to RestHandler.java (#947) Jul 21, 2021
@opensearch-ci-bot
Copy link
Collaborator

❌   DCO Check Failed ff992bc2d204eed64aba43cf60823a89ffdf8615
Run ./dev-tools/signoff-check.sh remotes/origin/1.x ff992bc2d204eed64aba43cf60823a89ffdf8615 to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success b55520865bdf838b654434742c5cce66839ab731

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success ff992bc2d204eed64aba43cf60823a89ffdf8615

@opensearch-ci-bot
Copy link
Collaborator

❌   DCO Check Failed b55520865bdf838b654434742c5cce66839ab731
Run ./dev-tools/signoff-check.sh remotes/origin/1.x b55520865bdf838b654434742c5cce66839ab731 to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success ff992bc2d204eed64aba43cf60823a89ffdf8615

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success b55520865bdf838b654434742c5cce66839ab731

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 9101f77

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 9101f77

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 783df77ff98a49e0bcb84593e83aa01f224036ee

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 783df77ff98a49e0bcb84593e83aa01f224036ee

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 9101f77

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 783df77ff98a49e0bcb84593e83aa01f224036ee

@dblock
Copy link
Member

dblock commented Jul 21, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 783df77ff98a49e0bcb84593e83aa01f224036ee
Log 342

Reports 342

@cliu123
Copy link
Member Author

cliu123 commented Jul 21, 2021 •
edited
Loading

@dblock @saratvemulapalli Could we have anyone review this backport PR? One of the open PRs on security plugin needs this PR to be merged. Thanks!

@cliu123 cliu123 mentioned this pull request Jul 21, 2021
Closed
@vrozov
Copy link
Contributor

vrozov commented Jul 21, 2021

@dblock Should not that PR be split into 2 separate PRs the same way how that was done in main?

dblock
dblock approved these changes Jul 21, 2021
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I'm ok with these going together, as long as we're ok squashing them (the only way merges happen here).

@cliu123
Copy link
Member Author

cliu123 commented Jul 21, 2021

I'm ok with these going together, as long as we're ok squashing them (the only way merges happen here).

@dblock I think we're ok squashing them as long as the finally merged comit message will reflect both of the 2 PRs.

@cliu123
Copy link
Member Author

cliu123 commented Jul 21, 2021

@dblock Are we waiting for anything to merge it?

@vrozov
Copy link
Contributor

vrozov commented Jul 22, 2021

IMO, those commits should not be squashed as they address two different issues.

@dblock
Copy link
Member

dblock commented Jul 22, 2021 •
edited
Loading

I am going to change my mind, I think @vrozov is right, @cliu123 would you please split these into two backport PRs? sorry for the annoyance

@cliu123 cliu123 changed the title [Backport-1.x]Avoid override of routes() in BaseRestHandler to respect the default behavior defined in RestHandler (#889) & Introduce replaceRoutes() method and 2 new constructors to RestHandler.java (#947) [Backport-1.x]Avoid override of routes() in BaseRestHandler to respect the default behavior defined in RestHandler (#889) Jul 22, 2021
@cliu123
Copy link
Member Author

cliu123 commented Jul 22, 2021 •
edited
Loading

I am going to change my mind, I think @vrozov is right, @cliu123 would you please split these into two backport PRs? sorry for the annoyance

@dblock Removed the 2nd commit from this PR. Once this PR get merged. I'll PR the other commit using the same branch.

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 9101f77

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 9101f77

@dblock
Copy link
Member

dblock commented Jul 22, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 9101f77

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 9101f77
Log 345

Reports 345

@dblock dblock merged commit 34def58 into opensearch-project:1.x Jul 22, 2021
@dblock
Copy link
Member

dblock commented Jul 22, 2021

Merged, thx

@cliu123
Copy link
Member Author

cliu123 commented Jul 22, 2021

@dblock Thanks for reviewing and merging! This is the PR for another commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cliu123 @opensearch-ci-bot @dblock @vrozov