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

Fix violations of non-constant format strings linter #51812

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

rosstimothy
Copy link
Contributor

Depends on https://github.com/gravitational/teleport.e/pull/6006. Now that we are compliant, the ignore rule was removed from the golangci-lint config to prevent future regressions.

@rosstimothy rosstimothy added the no-changelog Indicates that a PR does not require a changelog entry label Feb 3, 2025
@rosstimothy rosstimothy force-pushed the tross/trace_formatting branch 3 times, most recently from 6830eec to 2ada614 Compare February 4, 2025 15:10
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Many thanks, I confess I hesitated fixing this one when I saw how many occurrences there were.

I would swap all trace.Foo("%s", err.Error()) calls for just trace.Foo("%s", err). In practice it makes little difference, but I think the latter is the better idiom (and this will be heavily copied).

Various helpers have awful signatures like ConvertFoo(err error, args ...any) error, which begs the question of what one is supposed to do with args. I suggest cutting args and pushing whatever extra-information to the caller instead - they can "%w" or trace.Wrap or something else.

@rosstimothy rosstimothy force-pushed the tross/trace_formatting branch from 2ada614 to e6c6df2 Compare February 4, 2025 15:25
@rosstimothy rosstimothy marked this pull request as ready for review February 4, 2025 15:26
@github-actions github-actions bot added application-access audit-log Issues related to Teleports Audit Log database-access Database access related issues and PRs kubernetes-access machine-id size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Feb 4, 2025
@creack
Copy link
Member

creack commented Feb 4, 2025

Not a big fan if all the trace.Foo("%s", err) as there is no way to unwrap. May be interesting to add %w support in trace, but out of scope here.

@rosstimothy
Copy link
Contributor Author

Not a big fan if all the trace.Foo("%s", err) as there is no way to unwrap. May be interesting to add %w support in trace, but out of scope here.

Agreed. We need to make some time to reimagine trace to better interoperate with Go's changes to error wrapping.

@tigrato
Copy link
Contributor

tigrato commented Feb 4, 2025

Not a big fan if all the trace.Foo("%s", err) as there is no way to unwrap. May be interesting to add %w support in trace, but out of scope here.

Agreed. We need to make some time to reimagine trace to better interoperate with Go's changes to error wrapping.

But this PR doesn't change anything. Prior we would just fmt.Sprinf(err) 😿

@codingllama
Copy link
Contributor

Not a big fan if all the trace.Foo("%s", err) as there is no way to unwrap. May be interesting to add %w support in trace, but out of scope here.

Agreed. We need to make some time to reimagine trace to better interoperate with Go's changes to error wrapping.

https://pkg.go.dev/github.com/codingllama/semerr#pkg-overview (there's only so much I can resist.)

Depends on gravitational/teleport.e#6006.
Now that we are compliant, the ignore rule was removed from the
golangci-lint config to prevent future regressions.
@rosstimothy rosstimothy force-pushed the tross/trace_formatting branch from 8809325 to f0b8967 Compare February 4, 2025 16:04
@rosstimothy rosstimothy enabled auto-merge February 4, 2025 16:14
@rosstimothy rosstimothy added this pull request to the merge queue Feb 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 4, 2025
@rosstimothy rosstimothy added this pull request to the merge queue Feb 4, 2025
Merged via the queue into master with commit 2cced62 Feb 4, 2025
44 checks passed
@rosstimothy rosstimothy deleted the tross/trace_formatting branch February 4, 2025 17:04
carloscastrojumo pushed a commit to carloscastrojumo/teleport that referenced this pull request Feb 19, 2025
…1812)

Depends on https://github.com/gravitational/teleport.e/pull/6006.
Now that we are compliant, the ignore rule was removed from the
golangci-lint config to prevent future regressions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-access audit-log Issues related to Teleports Audit Log database-access Database access related issues and PRs kubernetes-access machine-id no-changelog Indicates that a PR does not require a changelog entry size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants