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

Update logging calls for upcoming Go 1.24 #2000

Merged
merged 2 commits into from
Jan 22, 2025
Merged

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jan 19, 2025

Some calls to Logger.PushPrefix and Logger.Info were being passed a single, variable argument, rather than a format string as the first argument (as printf-style functions expect).

The Vet in Go's upcoming 1.24 release has a new check that will flag these calls (which were already annotated with // nolint:govet comments). Rather than disabling linting, add a format-string first argument to make the calls kosher.

The Vet in Go 1.24 will add a new check to the printf analyzer that
"reports a diagnostic for calls of the form `fmt.Printf(s)`, where
`s` is a non-constant format string, with no other arguments.
Such calls are nearly always a mistake as the value of `s` may contain
the `%` symbol; use `fmt.Print` instead."
The Vet in Go 1.24 will add a new check to the printf analyzer that
"reports a diagnostic for calls of the form `fmt.Printf(s)`, where
`s` is a non-constant format string, with no other arguments.
Such calls are nearly always a mistake as the value of `s` may contain
the `%` symbol; use `fmt.Print` instead."
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jan 19, 2025

After these changes, internal/ contains only a single remaining // nolint comment. (It's here:)

// Cases where there is file there
case !regular:
return fmt.Errorf("error creating file %q: A non regular file exists there already and overwrite is false", f.Path)
case f.Contents.Source != nil:
return fmt.Errorf("error creating file %q: A file exists there already and overwrite is false", f.Path)
case regular && f.Contents.Source == nil:
break
default:
// nolint:staticcheck
return fmt.Errorf("Ignition encountered an internal error processing %q and must die now. Please file a bug", f.Path)
}

@prestist
Copy link
Collaborator

@ferdnyc Thank you for your contributions! and being on top of that, would you mind just squashing both commits into 1 and including the change for internal?

I think we could just do something like this wdyt?

return fmt.Errorf("%s", fmt.Sprintf("Ignition encountered an internal error processing %q and must die now. Please file a bug", f.Path))

@prestist prestist added the skip-notes This PR does not need release notes label Jan 21, 2025
@travier
Copy link
Member

travier commented Jan 22, 2025

I think we could just do something like this wdyt?

return fmt.Errorf("%s", fmt.Sprintf("Ignition encountered an internal error processing %q and must die now. Please file a bug", f.Path))

Hum, I don't think that nolint comment is for the format string here. It appears to be for something else.

Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

LGTM

@prestist
Copy link
Collaborator

Ah, okay, well then we can just keep it as is.

Copy link
Collaborator

@prestist prestist left a comment

Choose a reason for hiding this comment

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

LGTM, and thank you!

@prestist prestist merged commit 26ab8e3 into coreos:main Jan 22, 2025
9 of 10 checks passed
@ferdnyc ferdnyc deleted the go-124-compat branch January 22, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-notes This PR does not need release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants