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

fix: update generate_versions.go #2252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slashpai
Copy link
Member

Follow-up of rhobs/syncbot#72

  • I added CHANGELOG entry for this change.
  • [ x] No user facing changes, so no entry in CHANGELOG was needed.

Signed-off-by: Jayapriya Pai <janantha@redhat.com>
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2024
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

/lgtm

@simonpasquier
Copy link
Contributor

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Jan 31, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2024
@jan--f
Copy link
Contributor

jan--f commented Jan 31, 2024

/lgtm
/retest

Copy link
Contributor

openshift-ci bot commented Jan 31, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan--f, simonpasquier, slashpai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [jan--f,simonpasquier,slashpai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

openshift-ci bot commented Jan 31, 2024

@slashpai: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/versions cc696af link false /test versions

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@@ -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

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.

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

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

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 :))

@machine424
Copy link
Contributor

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2024
@rexagod
Copy link
Member

rexagod commented Mar 13, 2024

JFYI This needs make versions generate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants