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

Add GHA to update k3s-versions.json #7917

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

jandubois
Copy link
Member

@jandubois jandubois commented Dec 11, 2024

Closes #7901

The GHA is a copy of rddepman.yaml. It just runs scripts/k3s-versions.sh instead of yarn rddepman.

Sample PR generated by running k3s-versions.sh locally (and adding --repo jandubois/rancher-desktop to the gh pr create command): jandubois#12

Comment on lines 28 to 33
with:
fetch-depth: 0
Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

scripts/k3s-versions.sh Outdated Show resolved Hide resolved
@gunamata gunamata marked this pull request as draft December 11, 2024 17:53
@jandubois jandubois force-pushed the k3s-versions branch 2 times, most recently from 2f93254 to 9c51380 Compare December 11, 2024 22:20
@jandubois jandubois marked this pull request as ready for review December 11, 2024 22:22
@jandubois jandubois requested a review from mook-as December 11, 2024 22:28
.github/workflows/k3s-versions.yaml Show resolved Hide resolved
Comment on lines 28 to 33
with:
fetch-depth: 0
Copy link
Contributor

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.

scripts/k3s-versions.sh Show resolved Hide resolved
scripts/k3s-versions.sh Outdated Show resolved Hide resolved
scripts/k3s-versions.sh Outdated Show resolved Hide resolved
@jandubois jandubois force-pushed the k3s-versions branch 9 times, most recently from b81673c to f7b6ced Compare December 12, 2024 19:38
@jandubois jandubois requested a review from mook-as December 23, 2024 20:21
Copy link
Contributor

@mook-as mook-as left a 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 Show resolved Hide resolved
Comment on lines 28 to 33
with:
fetch-depth: 0
Copy link
Contributor

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.

scripts/k3s-versions.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@mook-as mook-as left a 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.

func getK3sChannels() (map[string]string, error) {
resp, err := http.Get("https://update.k3s.io/v1-release/channels")
if err != nil {
return nil, err
Copy link
Contributor

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:

Suggested change
return nil, err
return nil, fmt.Errorf("failed to get channel list: %w", err)

(Similarly for the other cases of returning errors.)

}
// Turn "v1.31.3+k3s1" into "1.31.3"
latest := strings.TrimPrefix(channel.Latest, "v")
latest = strings.Split(latest, "+")[0]
Copy link
Contributor

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?

Comment on lines 147 to 150
var versions []string
for _, version := range releaseMap {
versions = append(versions, version)
}
Copy link
Contributor

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?

Comment on lines 152 to 154
sort.Slice(versions, func(i, j int) bool {
return semver.Compare(versions[i], versions[j]) < 0
})
Copy link
Contributor

Choose a reason for hiding this comment

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

scripts/k3s-versions.go Outdated Show resolved Hide resolved
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))
Copy link
Contributor

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.

Copy link
Contributor

@mook-as mook-as left a 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.

Comment on lines 151 to 153
if len(os.Args) > 1 {
minimumVersion = os.Args[1]
}
Copy link
Contributor

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.

Copy link
Member Author

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.

@mook-as mook-as enabled auto-merge December 24, 2024 00:51
@mook-as mook-as merged commit 48d0bc9 into rancher-sandbox:main Dec 24, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GitHub action to update resources/k3s-versions.json
2 participants