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.

Sign up for GitHub

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

Jump to bottom

fix: update generate_versions.go #2252

Closed
wants to merge 1 commit into from
Closed

fix: update generate_versions.go #2252

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions hack/go/generate_versions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ import (
)

const (
mainBranch = "master"
metricsServerRepo = "openshift/kubernetes-metrics-server"
versionFile = "../../jsonnet/versions.yaml"
mainBranch = "master"
metricsServerRepo = "openshift/kubernetes-metrics-server"
versionFile = "../../jsonnet/versions.yaml"
versionNotFound = "N/A"
OCPVersionHeader = " OCP Version"
OCPVersionHeader = " OCP Version"
depsVersionsFile = "../../Documentation/deps-versions.md"
versionFileComments = `---
# This file is meant to be managed by hack/go/generate_versions.go script
Expand Down Expand Up @@ -146,7 +146,7 @@ func updateDepsVersionsFile(fileP string, components Components) error {
}
rows = append(rows, row)
err = releaseVersion.IncrementMinor()
if err != nil{
if err != nil {
return err
}
}
Expand Down Expand Up @@ -181,7 +181,7 @@ func getVersion(repo, ref string) (string, error) {
baseURL := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s", repo, ref)
link := fmt.Sprintf("%s/VERSION", baseURL)
if repo == metricsServerRepo {
link = fmt.Sprintf("%s/manifests/release/kustomization.yaml", baseURL)
link = fmt.Sprintf("%s/manifests/components/release/kustomization.yaml", baseURL)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid we'll have to branch as before v0.7.0, the file was in fact at /manifests/release/kustomization.yaml: https://github.com/kubernetes-sigs/metrics-server/blob/v0.6.4/manifests/release/kustomization.yaml

we started using v0.7.0 in 4.16, so we can branch on that.
Or fallback to the other path on a 404, as you want :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't check, but maybe we could add a VERSION file in our downstream?
syncbot should know the tag, we could just ask to put it there...

Copy link
Member

@rexagod rexagod Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah! That makes sense, I missed this! This will definitely break jobs on <4.16 PRs, true. This sounds like a good long-term approach, but would adding a conditional for checking both paths suffice here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes let's branch to unblock the CI, we'll try the other approach later.

Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, if https://github.com/openshift/release/blob/master/ci-operator/config/openshift/cluster-monitoring-operator/openshift-cluster-monitoring-operator-release-4.16.yaml and https://github.com/openshift/release/blob/master/ci-operator/config/openshift/cluster-monitoring-operator/openshift-cluster-monitoring-operator-release-4.15.yaml are sourced from the respective branches instead of master, wouldn't that automatically fix things for us?

Copy link
Member

Choose a reason for hiding this comment

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

(since this commit won't go into 4.15 as it's out, and only 4.16 will be up-to-date with master?)

Copy link
Contributor

Choose a reason for hiding this comment

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

But with the code from master we fetch versions for 4.14 and 4.17, so it should be compatible with both.

Copy link
Member

@rexagod rexagod Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right, I was confused by that. Could you elaborate a bit on the branching approach, since we already have branches till 4.17 on both (I'm not sure what exactly the "branching" entails here)? I'd also like to reiterate the idea of having a conditional that checks both /manifests/release/kustomization.yaml and /manifests/components/release/kustomization.yaml for a 200, and goes with that, which would fix things on both fronts. We can drop the older path once the CMO versions using them correspond to EOL releases. WDYT?

Copy link
Contributor

@machine424 machine424 Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I wasn't explicit, by branching I mean "conditional branching": adding a condition on the version (we're talking about the same thing :))

}
raw, err := fetchVersion(link)
if err != nil {
Expand Down