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.

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

refactor: replace urso/sderr with stdlib errors #39839

Merged
merged 5 commits into from
Jun 10, 2024
Merged

refactor: replace urso/sderr with stdlib errors #39839

merged 5 commits into from
Jun 10, 2024

Conversation

kruskall
Copy link
Member

@kruskall kruskall commented Jun 9, 2024

Proposed commit message

Go 1.20 added multiple errors wrapping so we can migrate to stdlib errors and drop the additional dependency on github.com/urso/sderr

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@kruskall
refactor: replace urso/sderr with stdlib errors
58b78cc 
Go 1.20 added multiple errors wrapping so we can migrate to stdlib errors and drop
the additional dependency on github.com/urso/sderr
@kruskall kruskall requested review from a team as code owners June 9, 2024 16:17
@kruskall kruskall requested review from faec and leehinman June 9, 2024 16:17
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 9, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 9, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kruskall? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@kruskall
Copy link
Member Author

kruskall commented Jun 9, 2024

golangci-lint / lint failure is unrelated to this PR

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jun 10, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 10, 2024
efd6
efd6 reviewed Jun 10, 2024
View reviewed changes
filebeat/input/filestream/copytruncate_prospector.go Outdated Show resolved Hide resolved
go.mod
@@ -144,7 +144,7 @@ require (
github.com/stretchr/testify v1.9.0
github.com/tsg/go-daemon v0.0.0-20200207173439-e704b93fd89b
github.com/ugorji/go/codec v1.1.8
github.com/urso/sderr v0.0.0-20210525210834-52b04e8f5c71
github.com/urso/sderr v0.0.0-20210525210834-52b04e8f5c71 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

Does go mod why explain why this is still here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see the PR descrition:

can be dropped completely from the dep graph once elastic/go-concert#69 is merged

leehinman
leehinman reviewed Jun 10, 2024
View reviewed changes
x-pack/filebeat/input/entityanalytics/provider/okta/statestore.go
// FIXME: Use fmt.Errorf("multiple errors during statestore close: %w", errors.Join(err, rollbackErr))
// when go1.20 is supported.
err = sderr.WrapAll([]error{err, rollbackErr}, "multiple errors during statestore close")
err = fmt.Errorf("multiple errors during statestore close: %w", errors.Join(err, rollbackErr))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the to include rollbackErr in theerrors.Join here? We know rollbackErr == nil so won't it just get discarded?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will and I'm honestly confused as well since that's what the previous code was doing 😅

Copy link
Contributor

@efd6 efd6 Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It can just be done non-conditionally. I don't know what I was thinking.

		err = fmt.Errorf("multiple errors during statestore close: %w", errors.Join(err, s.tx.Rollback()))

@kruskall kruskall merged commit 3c9f4d9 into elastic:main Jun 10, 2024
105 of 106 checks passed
@kruskall kruskall deleted the refactor/drop-sderr branch June 10, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmacknz cmacknz cmacknz approved these changes

@leehinman leehinman leehinman approved these changes

@efd6 efd6 efd6 approved these changes

@faec faec Awaiting requested review from faec faec is a code owner automatically assigned from elastic/elastic-agent-data-plane

Assignees

@kruskall kruskall

Labels
enhancement Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kruskall @elasticmachine @cmacknz @leehinman @efd6 @pierrehilbert