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

Prepare v1.15 stable branch #29838

Merged
merged 1 commit into from Dec 14, 2023
Merged

Prepare v1.15 stable branch #29838

merged 1 commit into from Dec 14, 2023

Conversation

joestringer
Copy link
Member

Requires:

The main differences for the branch prep here compared to commit
4fae666 ("Prepare v1.14 stable branch") are:

  • Ariane simplified some of the per-branch CI maintenance, so we don't
    need to delete all the workflows hosted in the main branch any more;
  • Related, several of the conformance tests don't need to be deleted any
    more, as they can be triggered from the branch version of the workflow
  • There's a few new workflows this release.

@joestringer joestringer requested review from a team as code owners December 13, 2023 08:09
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Dec 13, 2023
@joestringer joestringer requested review from a team as code owners December 13, 2023 08:09
@joestringer joestringer requested review from joamaki, aanm and nbusseneau and removed request for a team December 13, 2023 08:09
@joestringer joestringer added the dont-merge/blocked Another PR must be merged before this one. label Dec 13, 2023
@joestringer joestringer force-pushed the pr/joe/1.15-stable branch 2 times, most recently from 7dd95a9 to c3ac70c Compare December 13, 2023 08:39
@joestringer joestringer requested a review from a team as a code owner December 13, 2023 08:39
@joestringer joestringer requested review from qmonnet and removed request for a team December 13, 2023 08:39
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Plus the other review I left before.

.github/workflows/lint-workflows.yaml Show resolved Hide resolved
icon: https://cdn.jsdelivr.net/gh/cilium/cilium@main/Documentation/images/logo-solo.svg
version: 1.15.0-pre.3
appVersion: 1.15.0-pre.3
kubeVersion: ">= 1.26.0-0"
Copy link
Member

Choose a reason for hiding this comment

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

We haven't disclaimed anywhere that we aren't supporting k8s < 1.26. Maybe we should.

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 we've made some kind of announcement. It will be part of the docs page though: https://docs.cilium.io/en/latest/network/kubernetes/compatibility/#k8scompatibility .

I guess this is feedback to think about how we communicate this more clearly? Not sure whether to go and find the relevant PR and just bump its release-note to major or do something else...?

aanm
aanm previously requested changes Dec 13, 2023
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

I think install/kubernetes/Makefile.values also needs some branch-specific changes.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Apart from the items raised by André, all seems in order to me. Thanks!

@joestringer
Copy link
Member Author

I think install/kubernetes/Makefile.values also needs some branch-specific changes.

Yeah, I caught this just between you starting the review and finishing the review. I fixed it up to match the similar branching commit for 1.14 stable.

@joestringer joestringer requested a review from a team as a code owner December 14, 2023 00:29
@joestringer joestringer removed the dont-merge/blocked Another PR must be merged before this one. label Dec 14, 2023
@joestringer
Copy link
Member Author

/test

@joestringer
Copy link
Member Author

I'm not sure if the /test works with Ariane until we've merged the Ariane config as part of this PR 🤔 . We may have to merge this PR first, then in the next PR figure out whether Ariane and the required workflows are all configured correctly.

@joestringer joestringer added the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Dec 14, 2023
@joestringer
Copy link
Member Author

Holding back the merge until we figure out the codeowners workflow. I'm somewhat inclined to just delete it since that's what we did for v1.14, but it's probably worth taking a slightly closer look to ensure it's not reporting about a real issue. I'll have to look at that later.

@joestringer
Copy link
Member Author

I also looked around to see if we missed anything during the similar stable branch prep commit last time, but I couldn't find anything else relevant to inform how to further modify this preparation commit.

(I also took notes since this is changing 60+ files, this will be better documented in the release repo branching guide steps for next time).

@nbusseneau
Copy link
Member

I'm not sure if the /test works with Ariane until we've merged the Ariane config as part of this PR 🤔 . We may have to merge this PR first, then in the next PR figure out whether Ariane and the required workflows are all configured correctly.

No need to merge first, Ariane picks up the config file from the PR branch since it's not on a fork but on upstream. However since the PR changed the trigger to /test-backport-1.15 this is what should be used :D

@nbusseneau
Copy link
Member

/test-backport-1.15

@joestringer
Copy link
Member Author

Only two workflows are failing:

  • Codeowners lint, which doesn't make sense on stable branches (see this thread above)
  • Mergeability, blocked by labels on the PR.

I will push a fresh version to drop the codeowners lint workflow to resolve that failure. Otherwise assuming sufficient codeowner review, this should be good to merge.

@joestringer
Copy link
Member Author

However since the PR changed the trigger to /test-backport-1.15 this is what should be used :D

🤦 thanks, it's easy to miss the obvious stuff when you think too hard.

@joestringer joestringer removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Dec 14, 2023
The main differences for the branch prep here compared to commit
4fae666 ("Prepare v1.14 stable branch") are:
- Ariane simplified some of the per-branch CI maintenance, so we don't
  need to delete all the workflows hosted in the main branch any more;
- Related, several of the conformance tests don't need to be deleted any
  more, as they can be triggered from the branch version of the workflow
- There's a few new workflows this release.
- Here I bumped the version back to the latest prerelease to simplify
  the upcoming RC preparation. This also has impacts on the Helm
  directory, which is autogenerated based on the version tag.

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer
Copy link
Member Author

I've addressed all of @aanm's comments. There are outstanding reviews from @cilium/sig-servicemesh and @cilium/sig-clustermesh , but the changes there are trivially replacing a branch name. @julianwiedmann informs me that there are a couple of main branch workflows that rely on having a v1.15 branch CI image, which doesn't exist until we merge this PR because the workflow changes to create those images exist in this PR. I think it makes sense to merge this at this point.

@joestringer joestringer merged commit e0dc326 into v1.15 Dec 14, 2023
40 checks passed
@joestringer joestringer deleted the pr/joe/1.15-stable branch December 14, 2023 07:44
- hf/main/**
- hf/v1.15/**
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe a question for @nbusseneau and @cilium/ci-structure , could we just add vX.Y matches into the main branch workflows for most of these, or do they need to exist on the stable branch?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can do it on main, however in this case we will only have a single version of the workflow and it must work for all branches.

If we don't expect much drift between build workflows it would be a good idea and would reduce backporting pressure as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

That'd be awesome. I had a variant on the same problem related to #27876 (& co.), because those workflows only exist on stable branches. Such workflows that are only on older releases complicate the stable branch creation workflow, making it more error prone.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @pippolo84 for context on this question regarding PR 27876 and similar for older branches. It would be easier from a release management perspective to avoid workflows that are only on stable branches, or where the stable branch workflow significantly diverges from main. As a release manager, when I create a new stable branch, I would prefer not to have to go through and understand every different workflow that exists on main vs. the last stable branch in order to properly prepare the new stable branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #29902 to explore this specifically for the call-backport-label-updater workflow. Assuming we figure out the right details there, we can perhaps use that example as an example of how to update these other workflows.

@joestringer
Copy link
Member Author

This PR missed adding the workflow for backport labels updater, because that only exists on older branches: #27876

/.gitattributes @cilium/tophat
/.github/ @cilium/contributing
/.github/renovate.json5 @cilium/github-sec @cilium/ci-structure
/.github/actions/ @cilium/github-sec @cilium/ci-structure
Copy link
Member Author

Choose a reason for hiding this comment

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

This line was incorrectly removed. #31139 will fix it in-tree.

@joestringer
Copy link
Member Author

Looks like we also need to update the trigger phrases for ariane in future versions of this PR: https://docs.cilium.io/en/v1.15/contributing/testing/ci/#triggering-platform-tests (note that /test-backport-1.15 is missing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants