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

Restore exception behavior for backwards compatibility #245

Merged
merged 7 commits into from
Jan 9, 2025

Conversation

kimpepper
Copy link
Collaborator

@kimpepper kimpepper commented Jan 8, 2025

Description

In #233 we unintentionally removed the legacy exception behaviour.

This PR:

  • Restores the legacy exception behaviour and throws a deprecation warning when used
  • Renames ClientIntegrationTest to LegacyClientIntegrationTest
  • Adds a new ClientIntegrationTest that tests the new PSR client integration
  • Adds a Docker Compose file for local testing

Issues Resolved

Restores the legacy exception behaviour and throws a deprecation warning when used.

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.

Signed-off-by: Kim Pepper <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
@dblock
Copy link
Member

dblock commented Jan 8, 2025

Do we understand why these tests broke, meaning was that expected regressions? Iterate to green?

@dblock
Copy link
Member

dblock commented Jan 8, 2025

Merge #243 on top of this and it should pass, right?

@kimpepper kimpepper changed the title Fix outdated tests Restore exception behavior for backwards compatibility Jan 9, 2025
@kimpepper kimpepper force-pushed the fix-tests branch 2 times, most recently from 9f4e0ce to 5255508 Compare January 9, 2025 06:17
Signed-off-by: Kim Pepper <[email protected]>
shyim
shyim previously approved these changes Jan 9, 2025
Signed-off-by: Kim Pepper <[email protected]>
@kimpepper
Copy link
Collaborator Author

I've generated the API as a separate commit so reviewers can see the changes without the noise.

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 going to merge this and open an issue for backwards compatible exception behavior.

/**
* @deprecated in 2.3.2 and will be removed in 3.0.0.
*/
private bool $throwExceptions = false;
Copy link
Member

Choose a reason for hiding this comment

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

Was the old behavior true? In which case this is the wrong default.

We need something in the user guide about error handling.

@dblock dblock merged commit 6ed0301 into opensearch-project:main Jan 9, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants