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

refactor: replace urso/sderr with stdlib errors #69

Merged
merged 4 commits into from
Jun 18, 2024

Conversation

kruskall
Copy link
Member

@kruskall kruskall commented Jun 9, 2024

Go 1.20 added support for wrapping multiple errors so we can migrate to native errors and drop the additional dependency on github.com/urso/sderr. This allows beats and other downstream users to drop sderr as well.

Go 1.20 added support for wrapping multiple errors so we can migrate to
native errors and drop the additional dependency on github.com/urso/sderr.
This allows beats and other downstream users to drop sderr as well.
the go.mod uses a go 1.20 directive so go 1.19 is not supported
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jun 10, 2024
@@ -12,7 +12,6 @@ steps:
matrix:
setup:
go_version:
- "1.19.5"
- "1.20.5"
Copy link

Choose a reason for hiding this comment

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

How about updating to the actively maintained go versions go 1.21 and 1.22?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer for it to go in a separate PR along with bumping the go version to 1.21

Copy link

Choose a reason for hiding this comment

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

SGTM

@kruskall
Copy link
Member Author

test failure is uinrelated to this PR and already happening in main

belimawr
belimawr previously approved these changes Jun 13, 2024
@kruskall
Copy link
Member Author

@belimawr Thanks for the review! 🙇

fwiw it seems I don't have permission to merge this :(

@belimawr
Copy link

@belimawr Thanks for the review! 🙇

fwiw it seems I don't have permission to merge this :(

CI is failing on Windows, maybe that's why you can't merge. Could you take a look?

@kruskall
Copy link
Member Author

kruskall commented Jun 13, 2024

I don't think that't the reason, the button is missing and I see

Only those with write access to this repository can merge pull requests.

CI seems to be broken on main too (same test) so it looks unrelated to my changes

@belimawr
Copy link

I don't think that't the reason, the button is missing and I see

Only those with write access to this repository can merge pull requests.

CI seems to be broken on main too (same test) so it looks unrelated to my changes

Indeed. I rather fix main first and then merge this PR. Do you think you could take a look at the failure?

use atomic.Int64 to avoid race condition and make sure to increment
the counter before closing the channel.
use forever (~1h) duration to ensure the timeout/interval is ignored if
fn does not returns an error or a canceled context is passed (test 1 and 3).
use a large period to ensure the task doesn't stop waiting before the ctx
expired.
@kruskall
Copy link
Member Author

kruskall commented Jun 17, 2024

@belimawr Sure! 😄

I've pushed some changes, they should address the failing tests.

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@kruskall kruskall requested a review from belimawr June 18, 2024 14:06
@kruskall kruskall merged commit a21b699 into elastic:main Jun 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants