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

deps: bump various opentelemetry modules and golangci-lint #7175

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Nov 14, 2024

This release had a fix to avoid "superfluous call to WriteHeader" log lines:

https://github.com/open-telemetry/opentelemetry-go-contrib/releases/tag/v1.30.0

And there are further fixes in the releases we hadn't pulled in. Feels like a good idea to update this once in a while.


Re: Golangci-lint

The previous version has been failing without any good reason for me,
so let's try this one.

About the version pick: It's not the latest version (v1.62.0 at the
moment), because that would introduce a new revive rule,
redeclares-builtin-id, and that flags every variable called min or
max in the code base. I had started addressing these, but they were
just too many. Also I hadn't found a working way to disable said rule.

The new issues related to this version are mostly that it complains
whenever it finds a non-static string that makes its way into a printf-
like function. However, that's a common pattern in some place here, so
I've sprinkled some //nolint:govet on it.


When this goes in, please rebase, to keep the two commits separate.

Copy link

netlify bot commented Nov 14, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 39e9f68
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/67364cdaf77b8300088b80d5
😎 Deploy Preview https://deploy-preview-7175--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@srenatus srenatus force-pushed the sr/bump-otel branch 7 times, most recently from db988ec to 1955470 Compare November 14, 2024 14:10
@srenatus srenatus changed the title deps: bump various opentelemetry modules deps: bump various opentelemetry modules and golangci-lint Nov 14, 2024
@srenatus srenatus marked this pull request as ready for review November 14, 2024 14:19
@@ -1,6 +1,9 @@
run:
timeout: 5m

issues:
max-same-issues: 0 # don't hide issues in CI runs because they are the same type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a small quality-of-life improvement, I hope: Previously, if the linter found a total of 5 issues, but they all had the same type, it wouldn't show you all of them. Now, we ask it to just spit it all out, all at once.

@@ -28,7 +28,7 @@ ifeq ($(WASM_ENABLED),1)
GO_TAGS = -tags=opa_wasm
endif

GOLANGCI_LINT_VERSION := v1.59.1
GOLANGCI_LINT_VERSION := v1.60.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the aforementioned middle-ground: It works with our go version, but doesn't bring in too many new rules.

@@ -392,7 +392,7 @@ func eval(args []string, params evalCommandParams, w io.Writer) (bool, error) {
for _, t := range timers {
val, ok := t[name].(int64)
if !ok {
return false, fmt.Errorf("missing timer for %s" + name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not all bad! this is a good find

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @srenatus!

go.mod Show resolved Hide resolved
This release had a fix to avoid "superfluous call to WriteHeader" log lines:

https://github.com/open-telemetry/opentelemetry-go-contrib/releases/tag/v1.30.0

And there are further fixes in the releases we hadn't pulled in. Feels like
a good idea to update this once in a while.

Signed-off-by: Stephan Renatus <[email protected]>
The previous version has been failing without any good reason for me,
so let's try this.

About the version pick: It's not the latest version (v1.62.0 at the
moment), because that would introduce a new revive rule,
redeclares-builtin-id, and that flags every variable called `min` or
`max` in the code base. I had started addressing these, but they were
just too many.

The new issues related to this version are mostly that it complains
whenever it finds a non-static string that makes its way into a printf-
like function. However, that's a common pattern in some place here, so
I've sprinkled some nolint:govet on it.

Signed-off-by: Stephan Renatus <[email protected]>
@srenatus srenatus merged commit 20885fe into open-policy-agent:main Nov 14, 2024
28 checks passed
@srenatus srenatus deleted the sr/bump-otel branch November 14, 2024 19:36
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