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

Change minimal Go version to 1.21 and removed unnecessary test #5

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

edmocosta
Copy link
Collaborator

@edmocosta edmocosta commented Jul 24, 2024

  • Changed minimal go version to 1.21 (stable version), so other libraries does not need to bump their Go versions to use it.
  • Considering this library is not using (and probably won't use in the future) Goroutines, we can safety remove the package_test Goroutines leakage checks and avoid adding unneeded external decencies to the library.

@edmocosta edmocosta requested a review from a team as a code owner July 24, 2024 13:43
Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

There is no reason given in the PR or the commit for the go version downgrade and removal of the test, can we add that to the commit message?

@edmocosta
Copy link
Collaborator Author

There is no reason given in the PR or the commit for the go version downgrade and removal of the test, can we add that to the commit message?

Hey @jsvd, I've added it on the PR description! thanks for reviewing :)

@edmocosta edmocosta requested a review from jsvd July 24, 2024 15:56
Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I noticed you bumped .go-version again, maybe we could also bump versions in .golangci.yml so the library is tested with the most recent golang, even if it supports an older one.

@edmocosta
Copy link
Collaborator Author

Overall LGTM, I noticed you bumped .go-version again, maybe we could also bump versions in .golangci.yml so the library is tested with the most recent golang, even if it supports an older one.

I've adapted the .golangci.yml from another Elastic project, but it seems those go: ... versions on the linters have no effect, it should be on the run section instead. When it's empty, it defaults to the go.mod version and/or GOVERSION env var, which I think is the optimal behaviour for the linters. It also had a few invalid/deprecate settings that I fixed on the last commit.

@edmocosta edmocosta merged commit ee36186 into main Jul 25, 2024
1 check passed
@edmocosta edmocosta deleted the change-minimal-go branch July 25, 2024 11:35
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.

2 participants