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

Move error structs from opensearchapi to opensearch to be reused by plugins #512

Merged
merged 10 commits into from
Apr 8, 2024

Conversation

Jakob3xD
Copy link
Collaborator

@Jakob3xD Jakob3xD commented Apr 5, 2024

Description

  • Move error structs from opensearchapi to opensearch including tests
  • Add new errors structs
  • Move parseError function from opensearchapi to opensearch error.go as ParseError
  • Change ParseError function to do type assertion to determine the correct error type
  • Adjust tests to new errors
  • Handle non json errors

This is a braking change as the error types are moved to another project and also get renamed.
Needed as preparation for plugin integrations.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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.

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.70%. Comparing base (06a6dc8) to head (c619002).

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #512       +/-   ##
===========================================
+ Coverage   57.29%   77.70%   +20.41%     
===========================================
  Files         315       13      -302     
  Lines        9823     1673     -8150     
===========================================
- Hits         5628     1300     -4328     
+ Misses       2902      296     -2606     
+ Partials     1293       77     -1216     
Flag Coverage Δ
integration ?
unit 77.70% <100.00%> (+64.85%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
error.go 100.00% <100.00%> (ø)

... and 304 files with indirect coverage changes

@Jakob3xD Jakob3xD force-pushed the jh-errors branch 2 times, most recently from 720870c to a85e25b Compare April 5, 2024 18:44
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
… ParseError

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
…stom error

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
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.

Looks like a publicly-facing backwards-incompatible breaking change? Unless you want to inherit these error types in old names and mark those deprecated, let's bump the version to 4 as part of it?

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
@Jakob3xD
Copy link
Collaborator Author

Jakob3xD commented Apr 8, 2024

Looks like a publicly-facing backwards-incompatible breaking change? Unless you want to inherit these error types in old names and mark those deprecated, let's bump the version to 4 as part of it?

I adjusted the upgrading.md to version 4.0.0. Do you want the release done in this PR or after all my other PRs are merged?
I'd prefer after all the others are merged.

@dblock
Copy link
Member

dblock commented Apr 8, 2024

Looks like a publicly-facing backwards-incompatible breaking change? Unless you want to inherit these error types in old names and mark those deprecated, let's bump the version to 4 as part of it?

I adjusted the upgrading.md to version 4.0.0. Do you want the release done in this PR or after all my other PRs are merged? I'd prefer after all the others are merged.

After. Usually the version number should be set on working code as early as possible so that nobody confuses it with released code of the previous version.

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.

Merging this.

Would it make sense to make some of things like ParseError or UnmarshalJSON private only?

@dblock dblock merged commit 17956a2 into opensearch-project:main Apr 8, 2024
53 checks passed
@Jakob3xD Jakob3xD deleted the jh-errors branch April 9, 2024 07:38
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.

None yet

2 participants