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

Merged urls to new and old releases of calicoctl in a single link #9393

Closed

Conversation

JiffsMaverick
Copy link
Contributor

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

Fix errors during validating mirrors of calicoctl.

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 16, 2022
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JiffsMaverick
Once this PR has been reviewed and has the lgtm label, please assign liupeng0518 for approval by writing /assign @liupeng0518 in a comment. For more information see:The Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from EppO and jayonlau October 16, 2022 21:43
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 16, 2022
@oomichi
Copy link
Contributor

oomichi commented Oct 17, 2022

/ok-to-test
/cc @cristicalin

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 17, 2022
@@ -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 }}"
Copy link
Member

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

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Member

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A similar solution (version filter) is already used in several places: one, two, three.

Copy link
Member

Choose a reason for hiding this comment

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

@oomichi WDYT ?

Copy link
Contributor

@oomichi oomichi Nov 16, 2022

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

Copy link
Member

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oomichi

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

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

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2022
@floryut
Copy link
Member

floryut commented Dec 29, 2022

Superseded by #9515

@floryut floryut closed this Dec 29, 2022
@JiffsMaverick JiffsMaverick deleted the calicoctl-url-fix branch March 2, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants