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.

Sign up for GitHub

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

Jump to bottom

Allowing downloadable checksums and different checksum formats #1458

Merged
merged 1 commit into from
Sep 30, 2024
Merged

Allowing downloadable checksums and different checksum formats #1458

merged 1 commit into from
Sep 30, 2024

Conversation

huutomerkki
Copy link
Member

This PR changes most hardcoded checksums in this repo to downloadable ones. The exception is krew as there is no checksum available. Support for checksums provided in a checksums.txt file is also added.

Also renaming some variables so that if need comes, it'll be easier to add different checksum formats.

@metal3-io-bot metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 18, 2024
@huutomerkki huutomerkki force-pushed the minna/convert-hardcoded-checksums-to-downloadable branch from a5837e1 to 824f981 Compare September 19, 2024 05:25
@huutomerkki
Copy link
Member Author

/test metal3-dev-env-integration-test-ubuntu-main
/test metal3-centos-e2e-integration-test-release-1-8

@huutomerkki huutomerkki force-pushed the minna/convert-hardcoded-checksums-to-downloadable branch from 824f981 to e9d7bc3 Compare September 19, 2024 06:25
@huutomerkki
Copy link
Member Author

/retest

@huutomerkki
Copy link
Member Author

/test metal3-dev-env-integration-test-ubuntu-main
/test metal3-centos-e2e-integration-test-release-1-8

@huutomerkki huutomerkki force-pushed the minna/convert-hardcoded-checksums-to-downloadable branch from e9d7bc3 to 32a8064 Compare September 19, 2024 06:51
@huutomerkki
Copy link
Member Author

/test metal3-dev-env-integration-test-ubuntu-main
/test metal3-centos-e2e-integration-test-release-1-8

@huutomerkki
Copy link
Member Author

/retest

Rozzii
Rozzii reviewed Sep 19, 2024
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2024
tuminoid
tuminoid reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

A minor style nit, otherwise looks solid improvement.

Also,
/hold
for a sec to let me test the various scenarios. This seems to handle both direct and multi-arch files, so expecting a quick pass.

lib/download.sh Outdated Show resolved Hide resolved
lib/download.sh Outdated Show resolved Hide resolved
@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2024
@huutomerkki huutomerkki force-pushed the minna/convert-hardcoded-checksums-to-downloadable branch from 32a8064 to 642fb42 Compare September 23, 2024 05:39
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2024
tuminoid
tuminoid reviewed Sep 23, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Whitespace nits + actual issues with Tilt verification.

lib/download.sh Outdated Show resolved Hide resolved
lib/download.sh Outdated Show resolved Hide resolved
lib/download.sh
checksum="$(curl -SsL "${checksum}")"
fi

if [[ "${checksum}" =~ ${dlname,,} ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

This does not work for Tilt.

  1. We download Tilt's installation script, and the checksum is for release tarball. It cannot find install.sh on the checksum file, and skips it.
  2. It does not return failure for this case, and happily goes and installs TIlt anyways.

We either need to change the download itself to the release tarball or use hardcoded SHA for the script. Using hardcoded SHA wouldn't be too bad, as it is the same script regardless of arch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I had missed that we use a different install method for tilt. I can change it back to the pinned SHA for now, but changing the installation method shouldn't be too hard either if we want that instead.

@huutomerkki huutomerkki force-pushed the minna/convert-hardcoded-checksums-to-downloadable branch from 642fb42 to dc86a7f Compare September 23, 2024 12:29
tuminoid
tuminoid reviewed Sep 23, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

One whitespace mishap, but this is good to merge otherwise.

/approve
/hold for the whitespace fix

lib/download.sh Outdated Show resolved Hide resolved
@metal3-io-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tuminoid

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

@metal3-io-bot metal3-io-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 23, 2024
@huutomerkki huutomerkki force-pushed the minna/convert-hardcoded-checksums-to-downloadable branch from dc86a7f to 05f6982 Compare September 30, 2024 05:40
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2024
@tuminoid
Copy link
Member

/test metal3-dev-env-integration-test-ubuntu-main metal3-centos-e2e-integration-test-release-1-8
/unhold

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2024
@kashifest
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2024
@metal3-io-bot metal3-io-bot merged commit bc435b3 into metal3-io:main Sep 30, 2024
5 checks passed
@metal3-io-bot metal3-io-bot deleted the minna/convert-hardcoded-checksums-to-downloadable branch September 30, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rozzii Rozzii Rozzii left review comments

@tuminoid tuminoid tuminoid left review comments

@dtantsur dtantsur Awaiting requested review from dtantsur

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@huutomerkki @metal3-io-bot @tuminoid @kashifest @Rozzii