-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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 Calico v3.17.3 and v3.18.1 #7524
Add Calico v3.17.3 and v3.18.1 #7524
Conversation
Hi @cristicalin. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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'm not following calico closely but why not 3.18.1 ? |
My rationale was to add that in a separate PR to keep review effort focused on 1 change at a time. I added the hashes for 3.18.1 as well in a separate commit but kept the default to 3.17.3 unless you think I should bump the default to 3.18.1 as well. One thing that I'm not fully sure is if to go from 3.16.9 to 3.18.1 we would need to go through 3.17.3 as an interim version, I have to do some testing in that area. |
cebc036
to
fa1026b
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.
/ok-to-test
Fine to upgrade only to 3.17.3, we could always bump to 3.18.x later on
I have a suspicion that the CI failure is unrelated (markdown job failed in a git fetch), could you re-trigger it? |
Relaunched, kubespray CI always need a bit of love to pass :P |
3c3a8ad
to
4f760e8
Compare
This time the issue was with Calico 3.17 and newer crd files missing the It turns out my hash for the 3.18 archive was also wrong so I also fixed it. @floryut can you bless another CI run? |
CI is running right now, no error for now 🙏 |
Again last failure seems unrelated to this change. |
@@ -119,12 +119,13 @@ | |||
-e '/^\s{2,4}annotations:/{:1;/\(devel\)$/!{N;b 1}; /.*/d}' | |||
{{ local_release_dir }}/calico-{{ calico_version }}-kdd-crds/*.yaml | |||
when: | |||
- calico_version is version('v3.17.0', '<') | |||
- calico_version is version('v3.19.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.
Are you sure about this one ?
I think those annotations are not present in 3.17+
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.
Indeed, I looked over the CRDs from 3.17 and 3.18 and they were not there. I just assumed that since there was no other mention of 3.17 before this patch that when condition was an upper bound. That said, the command itself is harmless since it will not change anything in the 3.17+ CRD files but I removed the unnecessary change anyway.
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 agree it's harmless but in kubespray we tend to carry workaround for way longer than needed, once we remove the 3.17 hash we can also remove this task ;)
/cc @champtar |
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.
see previous message
4ba1a43
to
72a809c
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: champtar, cristicalin, floryut 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:
Approvers can indicate their approval by writing |
we will have to wait for #7535 to get merged and maybe need rebase |
72a809c
to
0f72c03
Compare
Rebased after #7535 was merged as requested. |
/lgtm |
you forgot to update the README |
* add hashes for calico v3.17.3 * add hashes for claico v3.18.1 * bump default calico version to v3.17.3 * calico crds are missing yaml separator breaking kdd
* Add Calico v3.17.3 and v3.18.1 (kubernetes-sigs#7524) * add hashes for calico v3.17.3 * add hashes for claico v3.18.1 * bump default calico version to v3.17.3 * calico crds are missing yaml separator breaking kdd * Calico new versions v3.17.4 and v3.18.2 (kubernetes-sigs#7563) * calico: upgrade from v3.17.3 to v3.17.4 * calico: upgrade from v3.18.1 to v3.18.2 * Fixes issue kubernetes-sigs#7573 - Made Calico permissions compatible with v3.18.x (see projectcalico/calico#4557). Specifically, granted watch to custom resources blockaffinities, ipamblocks & ipamhandles (kubernetes-sigs#7575) * bump calico 3.18 to v3.18.3 (kubernetes-sigs#7592) * Support Calico advertisement of MetalLB LoadBalancer IPs (kubernetes-sigs#7593) * add initial MetalLB docs * metallb allow disabling the deployment of the metallb speaker * calico>=3.18 allow using calico to advertise service loadbalancer IPs * Document the use of MetalLB and Calico * clean MetalLB docs Co-authored-by: Cristian Calin <[email protected]> Co-authored-by: holmesb <[email protected]>
* add hashes for calico v3.17.3 * add hashes for claico v3.18.1 * bump default calico version to v3.17.3 * calico crds are missing yaml separator breaking kdd
* add hashes for calico v3.17.3 * add hashes for claico v3.18.1 * bump default calico version to v3.17.3 * calico crds are missing yaml separator breaking kdd
What type of PR is this?
What this PR does / why we need it:
This PR adds the necessary hashes to support Calico versions 3.17 and 3.18 (while bumping the default to 3.17.3).
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The release notes for versions 3.17 and 3.18 compared to the aging 3.16 are long so I include just the links here:
Does this PR introduce a user-facing change?: