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

Adapt checksums and versions to new hashes updater #11890

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

VannTen
Copy link
Contributor

@VannTen VannTen commented Jan 14, 2025

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This (WIP) adapt Kubespray checksums and version to work with the new hashes updater.
I'm pushing this as draft so reviewers of the hashes updaters can test it.

This is pretty invasive, as you can see, mostly because of removing the v-prefix in version (this originates from the design of the updater script, but I've been going with it for consistency).

Which issue(s) this PR fixes:

Special notes for your reviewer:
I'm putting this as ci/extended to avoid breaking stuff

This might be hard to backport, which might preclude automatic PR for hash updates on the (current) releases branches.

Does this PR introduce a user-facing change?:

action required
You should remove the leading 'v' of all explicit version of components deployed by kubespray (most notably `kube_version`)

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 14, 2025
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 14, 2025
@VannTen
Copy link
Contributor Author

VannTen commented Jan 14, 2025 via email

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 14, 2025
@VannTen
Copy link
Contributor Author

VannTen commented Jan 14, 2025

/label ci/extended

@k8s-ci-robot
Copy link
Contributor

@VannTen: The label(s) /label ci/extended cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, ci-short, ci-extended, ci-full. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label ci/extended

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-sigs/prow repository.

@VannTen VannTen marked this pull request as draft January 14, 2025 09:57
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2025
@VannTen
Copy link
Contributor Author

VannTen commented Jan 14, 2025

/label tide/merge-method-merge

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges. label Jan 14, 2025
@VannTen
Copy link
Contributor Author

VannTen commented Jan 14, 2025

/label ci-extended

@k8s-ci-robot k8s-ci-robot added the ci-extended Run additional tests label Jan 14, 2025
@VannTen
Copy link
Contributor Author

VannTen commented Jan 15, 2025

/retest

@VannTen VannTen force-pushed the download_graphql_checksums_2 branch from 59e8f8f to b2e8473 Compare January 15, 2025 15:15
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2025
@VannTen VannTen force-pushed the download_graphql_checksums_2 branch from b2e8473 to 0b9d9bb Compare January 16, 2025 10:12
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2025
@VannTen VannTen force-pushed the download_graphql_checksums_2 branch 2 times, most recently from bce30fc to 2d64093 Compare January 16, 2025 10:39
@VannTen VannTen force-pushed the download_graphql_checksums_2 branch 3 times, most recently from 6d54849 to 6619a62 Compare January 17, 2025 20:24
@VannTen VannTen force-pushed the download_graphql_checksums_2 branch from a3e081f to 815bc6e Compare January 20, 2025 13:21
@VannTen VannTen requested a review from mzaian January 20, 2025 14:42
Copy link
Contributor

@mzaian mzaian left a comment

Choose a reason for hiding this comment

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

Thanks @VannTen

@yankay
Copy link
Member

yankay commented Jan 23, 2025

It's a break change :-)
This is a breaking change. We should highlight it in the release notes.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2025
@VannTen
Copy link
Contributor Author

VannTen commented Jan 23, 2025 via email

@VannTen VannTen force-pushed the download_graphql_checksums_2 branch from 815bc6e to e60f68f Compare January 23, 2025 08:22
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2025
@VannTen
Copy link
Contributor Author

VannTen commented Jan 23, 2025

Hum, what do you think regarding the README versions ? Should we keep a leading v here or use the same consistency and get rid of it ?

Now that I'm writing it it seems the 2nd option seems to make more sense (I don't care much though, can change if needed)

@yankay
Copy link
Member

yankay commented Jan 23, 2025

Hum, what do you think regarding the README versions ? Should we keep a leading v here or use the same consistency and get rid of it ?

Now that I'm writing it it seems the 2nd option seems to make more sense (I don't care much though, can change if needed)

Hello @VannTen ,

This change may cause errors for users who are currently using Kubespray, especially those who use the kube_version variable.

It would be more user-friendly for existing users if we could provide compatibility with the previous v prefix pattern and provide some documentation to help users migrate to the new way.

@VannTen
Copy link
Contributor Author

VannTen commented Jan 23, 2025

I considered doing that.
However, unless we maintain infinite backwards compat, users will have to change their inventories as some point, so why not now ? To that effect, I include e60f68f, which explicitly state the version format change (would you prefer a more imperative tone ?). The change is super straightforward, so I don't see what a deprecation period would buy us in that case.

@yankay
Copy link
Member

yankay commented Jan 23, 2025

I considered doing that. However, unless we maintain infinite backwards compat, users will have to change their inventories as some point, so why not now ? To that effect, I include e60f68f, which explicitly state the version format change (would you prefer a more imperative tone ?). The change is super straightforward, so I don't see what a deprecation period would buy us in that case.

Because many existing clusters require maintenance, the compatibility may need to be maintained. It may be solved in another PR later.
Hope more reviewers can help us to review it and give some suggestions on it.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mzaian, VannTen, yankay

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 [VannTen,mzaian,yankay]

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

@VannTen
Copy link
Contributor Author

VannTen commented Jan 23, 2025 via email

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2025
With the refactored script, this would try to download those releases
each time.
The versions are nearly all .0 because of the gvisor release scheme.
This means they need to be quoted in yaml to be considered strings.
Special casing by removing the .0 make tooling more complicated, and it
does not gain us anything apart from a nicer looking file (I guess).

So just use the version of upstream gvisor and quote it.
also remove the version key in all dict in downloads since it's not used
anywhere.
Allow the use of different hashes, as support by the get_url
Ansible module.
Change the variable name accordingly to 'checksum' since it's not
exclusively sha256 anymore.
Only check version that we previously prefixed with 'v'.
This commit should be reverted as soon as 2.28 is released.
Also small spacing fix.
@VannTen VannTen force-pushed the download_graphql_checksums_2 branch from f07ec1d to 8a10d55 Compare January 24, 2025 08:14
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2025
@VannTen VannTen requested review from mzaian and yankay January 28, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. ci-extended Run additional tests cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants