-
Notifications
You must be signed in to change notification settings - Fork 55
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
RFE: ci: add go 1.19, 1.20, drop older versions #103
Conversation
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.
The logic seems reasonable to me, thanks @kolyshkin.
Acked-by: Paul Moore <paul@paul-moore.com>
@drakenclimber PTAL |
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.
I had one rather pointless comment, @kolyshkin; feel free to ignore it. This looks good to me. Thanks for the help.
seccomp.go
Outdated
// as an error return from the syscall that created the notification. | ||
// | ||
// syscall (e.g., syscall.EPERM, etc). In the latter case, it's used | ||
// as an error return from the syscall that created the notification. |
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 formatting feels weird to me, but I don't have a strong opinion. Let's make the tool happy, I guess.
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.
It is indeed weird, and it is not rendered the way I thought it would. Let me take a closer look; for now I'm converting this PR to draft.
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.
Fixed, ready for review/merge.
The reason gofumpt
did this is since a recent go release (1.18? I am not sure) indented lines are treated as code blocks, so gofumpt inserted an empty line and reformatted those "code blocks" using tab instead of spaces.
All this would make sense if those were really code blocks. They are not, and they are in fact misplaced, and thus are not rendered correctly (see e.g. https://pkg.go.dev/github.com/seccomp/libseccomp-golang#ScmpNotifData).
The fix is to move the docstrings to just before the IDs they describe, and remove the indentation. This is what the first commit now does.
1. A newer gofumpt misformats the documentation which is indented, such as the second line of this code: // Val: return value for the syscall that created the notification. Only // relevant if Error is 0. This happens because in recent Go versions (1.18?) this is treated as code blocks. This was definitely not an intention. 2. Godoc blocks should be placed right before the identifiers it documents. Solve both issues my moving the docstrings where they belong. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since go 1.20 is released, only 1.20.x and 1.19.x are supported, and all the older versions are not: - go 1.18.x is not supported since Go 1.20 release in Feb 2023; - go 1.17.x is not supported since Go 1.19 release in Aug 2022; - go 1.16.x is not support since Go 1.18 release in May 2022. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The meaning of this setting is somewhat vague in the official Go docs. To my best understanding, this value mostly influences how Go toolchain deals with go.mod and go.sum files. In our case, we don't use any other go modules, so go.mod and go.sum are quite shallow, yet I guess it makes sense to have something sensible in here. Switch from 1.14 to 1.19 as at the moment this is the oldest Go version that is still supported. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
... to the latest version. This may or may not be required with the latest Go. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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.
Looks good to me. Thanks @kolyshkin
Acked-by: Tom Hromatka <tom.hromatka@oracle.com>
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.
Looks good to me, thanks.
Acked-by: Paul Moore <paul@paul-moore.com>
v2 (Mar 9 2023): updated to add go 1.20.
Closes: #105