-
Notifications
You must be signed in to change notification settings - Fork 45
image: enable gofumpt formatter #457
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
Conversation
common and storage already use gofumpt. enable it for image based on the outcome from our community discussion[1]. And it makes this repo use the same formatter consistently which is nice. [1] containers/podman#27291 Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6493 |
mtrmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
(Note to self: I didn’t review all the changes, hopefully that can be verified by re-formatting and comparing.)
image/docker/daemon/daemon_dest.go
Outdated
| if sys != nil && sys.DockerDaemonHost != client.DefaultDockerHost { | ||
| mustMatchRuntimeOS = false | ||
| } | ||
| mustMatchRuntimeOS := sys != nil && sys.DockerDaemonHost != client.DefaultDockerHost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, yes I just copy pasted without thinking about it to much but true/false is inverted here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switched to sys == nil || sys.DockerDaemonHost == client.DefaultDockerHost which I think is the right logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. (It wouldn’t be too bad to do
connectsToDifferentHost := sys != nil && sys.DockerDaemonHost != client.DefaultDockerHost
mustMatchRuntimeOS := !connectsToDifferentHostto self-document what the decision is about.)
This was uncovered by the gofumpt changes it seems. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Add simple fmt target to run golangci-lint fmt to format the source tree. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Signed-off-by: Paul Holzinger <pholzing@redhat.com>
mtrmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let’s get this in quickly, this would be a pain to rebase.
Thanks!
common and storage already use gofumpt. enable it for image based on the
outcome from our community discussion[1]. And it makes this repo use the
same formatter consistently which is nice.
[1] containers/podman#27291