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

Missing:

  • Validating step to detect version starting with v (should be relatively straigtforward)

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 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
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.
@VannTen VannTen force-pushed the download_graphql_checksums_2 branch 4 times, most recently from 7fada99 to 616dd35 Compare January 18, 2025 13:46
@VannTen
Copy link
Contributor Author

VannTen commented Jan 18, 2025

/label tide/merge-method-merge

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.
@VannTen VannTen force-pushed the download_graphql_checksums_2 branch from 616dd35 to 4f55d36 Compare January 18, 2025 14:08
@VannTen VannTen force-pushed the download_graphql_checksums_2 branch from 4f55d36 to 5f650e4 Compare January 18, 2025 14:39
@VannTen
Copy link
Contributor Author

VannTen commented Jan 18, 2025

So, this is passing CI 🎉

What I want to add before merging this:

  • assert tasks to fail fast on version starting with 'v'

@VannTen VannTen marked this pull request as ready for review January 20, 2025 08:45
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 20, 2025
@VannTen VannTen force-pushed the download_graphql_checksums_2 branch 3 times, most recently from 92f52b1 to a3e081f Compare January 20, 2025 09:40
Only check version that we previously prefixed with 'v'.
This commit should be reverted as soon as 2.28 is released.
@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

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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.

3 participants