-
Notifications
You must be signed in to change notification settings - Fork 298
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
Add GHA to update k3s-versions.json #7917
Conversation
cb994c9
to
73a60b6
Compare
.github/workflows/k3s-versions.yaml
Outdated
with: | ||
fetch-depth: 0 |
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 don't think this is needed.
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 needed now because we may add a commit to an existing branch.
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 thought you could push new commits to branches from a shallow clone, because the parent commit has a known commit hash? I'll test this in my fork, 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.
Yeah, this is not needed; since the parent commit hash doesn't change, having the default fetch depth of 1 is totally fine.
2f93254
to
9c51380
Compare
9c51380
to
cbe2091
Compare
.github/workflows/k3s-versions.yaml
Outdated
with: | ||
fetch-depth: 0 |
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 thought you could push new commits to branches from a shallow clone, because the parent commit has a known commit hash? I'll test this in my fork, I guess.
b81673c
to
f7b6ced
Compare
f7b6ced
to
5102649
Compare
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.
Needed some changes to make it run.
I haven't reviewed the golang code in depth yet.
.github/workflows/k3s-versions.yaml
Outdated
with: | ||
fetch-depth: 0 |
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.
Yeah, this is not needed; since the parent commit hash doesn't change, having the default fetch depth of 1 is totally fine.
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.
Review of the golang code.
scripts/k3s-versions.go
Outdated
func getK3sChannels() (map[string]string, error) { | ||
resp, err := http.Get("https://update.k3s.io/v1-release/channels") | ||
if err != nil { | ||
return nil, err |
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 would be nice to have some context for the error:
return nil, err | |
return nil, fmt.Errorf("failed to get channel list: %w", err) |
(Similarly for the other cases of returning errors.)
scripts/k3s-versions.go
Outdated
} | ||
// Turn "v1.31.3+k3s1" into "1.31.3" | ||
latest := strings.TrimPrefix(channel.Latest, "v") | ||
latest = strings.Split(latest, "+")[0] |
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.
Not that it's needed, bu SplitN
?
scripts/k3s-versions.go
Outdated
var versions []string | ||
for _, version := range releaseMap { | ||
versions = append(versions, version) | ||
} |
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.
Is that roughly maps.Values
?
scripts/k3s-versions.go
Outdated
sort.Slice(versions, func(i, j int) bool { | ||
return semver.Compare(versions[i], versions[j]) < 0 | ||
}) |
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.
And that's roughly slices.SortedFunc(versions, semver.Compare)
?
scripts/k3s-versions.go
Outdated
minimumVersion = os.Args[1] | ||
} | ||
if !semver.IsValid(minimumVersion) { | ||
panic(fmt.Errorf("minimum version %q is not a valid version, e.g. needs to start with 'v'", minimumVersion)) |
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 may be easier to read to have an inner function that returns error
and have main
panic if the result is non-nil
.
5102649
to
1b7df67
Compare
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.
One very trivial comment.
scripts/k3s-versions.go
Outdated
if len(os.Args) > 1 { | ||
minimumVersion = os.Args[1] | ||
} |
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.
Do we still need this? Without it minimumVersion
can be a const
, I think. And we no longer need to check its validity.
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.
We don't really need it (and it was originally a const
). I only added it because while testing I ran it like this:
❯ go run scripts/k3s-versions.go v1.30
{
"cacheVersion": 2,
"channels": {
"latest": "1.31.4",
"stable": "1.31.4",
"v1.30": "1.30.8",
"v1.31": "1.31.4"
},
"versions": [
"v1.30.0+k3s1",
"v1.30.1+k3s1",
"v1.30.2+k3s2",
"v1.30.3+k3s1",
"v1.30.4+k3s1",
"v1.30.5+k3s1",
"v1.30.6+k3s1",
"v1.30.7+k3s1",
"v1.30.8+k3s1",
"v1.31.0+k3s1",
"v1.31.1+k3s1",
"v1.31.2+k3s1",
"v1.31.3+k3s1",
"v1.31.4+k3s1"
]
}
But I guess this is no longer a valid use case, so I will remove it.
1b7df67
to
b219a2d
Compare
Signed-off-by: Jan Dubois <[email protected]>
b219a2d
to
569cd1a
Compare
Closes #7901
The GHA is a copy of
rddepman.yaml
. It just runsscripts/k3s-versions.sh
instead ofyarn rddepman
.Sample PR generated by running
k3s-versions.sh
locally (and adding--repo jandubois/rancher-desktop
to thegh pr create
command): jandubois#12