-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Merged urls to new and old releases of calicoctl in a single link #9393
Merged urls to new and old releases of calicoctl in a single link #9393
Conversation
Hi @JiffsMaverick. 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JiffsMaverick The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/ok-to-test |
@@ -32,8 +32,7 @@ | |||
# etcd_download_url: "{{ files_repo }}/github.com/etcd-io/etcd/releases/download/{{ etcd_version }}/etcd-{{ etcd_version }}-linux-{{ image_arch }}.tar.gz" | |||
|
|||
# [Optional] Calico: If using Calico network plugin | |||
# calicoctl_download_url: "{{ files_repo }}/github.com/projectcalico/calico/releases/download/{{ calico_ctl_version }}/calicoctl-linux-{{ image_arch }}" | |||
# calicoctl_alternate_download_url: "{{ files_repo }}/github.com/projectcalico/calicoctl/releases/download/{{ calico_ctl_version }}/calicoctl-linux-{{ image_arch }}" | |||
# calicoctl_download_url: "{{ files_repo }}/github.com/projectcalico/{{ 'calico' if (calico_ctl_version is version('v3.21.3','>=')) else 'calicoctl' }}/releases/download/{{ calico_ctl_version }}/calicoctl-linux-{{ image_arch }}" |
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.
3.22.0 maybe better than the 3.21.3.
Every version <=3.21.x can be found at the https://github.com/projectcalico/calicoctl
Every version >=3.22.x can be found at the https://github.com/projectcalico/calico
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.
Honestly until we drop calico < 3.21 I would keep this because it was introduced specifically with calico breaking release artefacts mid-cycle. The errors you are seeing are aesthetic only and allow us to deploy older versions of calico.
/hold
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.
My PR also supports older versions of Calico, but it still removes the error. For new releases (>=3.21.3) uses the new repo url (without "ctl" in the end), for old releases (<3.21.3) uses the old url (with "ctl" at the end).
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 kind of agree that this PR might be nice but I'm not sure we need it.
Should we simply close it and remove the alternate url once <3.21.x support is dropped ?
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.
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.
@oomichi WDYT ?
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.
Sorry for my late comment.
I prefer current simple lines without a condition in the middle of the line.
In addition, calico 3.21 will be un-maintained soon according to https://github.com/projectcalico/calico/branches
So we might drop calico 3.21 simply in near future by removing calicoctl_alternate_download_url
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.
Closing then, are you align with this @JiffsMaverick ?
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.
When will Calico developers stop supporting this version? I didn't find any information about it on their website.
I'm just very interested in making sure there are no errors while the playbooks are running. Therefore, I want to know the date (or a way to understand that support is over) so that I can send a new pull request to fix these errors as soon as possible.
Our company maintains its fork of Kubespray with the changes we need. But we would like to have these fixes in the upstream and we would no longer need to maintain our fork :)
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'm just very interested in making sure there are no errors while the playbooks are running.
Yeah, I see that point.
When I faced some issue at Kubespray ansible-playbook run, I investigated the output errors.
And if the unrelated errors were output, that made the investigation difficult and mess.
It is very nice to reduce such unrelated errors as possible, I completely agree with you.
Therefore, I want to know the date (or a way to understand that support is over) so that I can send a new pull request to fix these errors as soon as possible.
OK, Calico v3.21 branch is not updated at all since 2 months ago.
And the version is our lowest supported version since #8935
So let me propose a pull request to drop the version.
BTW an issue #9514 is reported.
The other people also would be facing the same issue.
Superseded by #9515 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR solves errors during validation mirrors of calicoctl.
TASK [download : download_file | Validate mirrors] ********************************************************************************************************************** failed: [master-1] (item=None) => {"attempts": 4, "censored": "the output has been hidden due to the fact that 'no_log: true' was specified for this result", "changed": false} ok: [master-1] => (item=None) fatal: [master-1 -> {{ download_delegate if download_force_cache else inventory_hostname }}]: FAILED! => {"censored": "the output has been hidden due to the fact that 'no_log: true' was specified for this result", "changed": false} ...ignoring
Which issue(s) this PR fixes:
Special notes for your reviewer:
More info about different links for calicoctl and "Validate mirror" in #8474
Does this PR introduce a user-facing change?: