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

Fix krew on Apple silicon #837

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Fix krew on Apple silicon #837

merged 3 commits into from
Jan 10, 2023

Conversation

rgee0
Copy link
Contributor

@rgee0 rgee0 commented Jan 8, 2023

Signed-off-by: Richard Gee richard@technologee.co.uk

Description

Adds arm64 for darwin into the tool templates as #836 looks like a possible Apple silicon gap.
There was a darwin_arm64 entry under the ming path in the template, so the or has been added in there too.

Motivation and Context

  • I have raised an issue to propose this change, which has been given a label of design/approved by a maintainer

Might fix #836 - would have been more certain had @bayeslearner completed the issue template.

How Has This Been Tested?

Added unit tests but haven't been able to test as I don't have apple silicon. Maybe @bayeslearner can help? Here's how

Are you a GitHub Sponsor yet (Yes/No?)

  • Yes
  • No

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Documentation

  • I have updated the list of tools in README.md if (required) with ./arkade get -o markdown
  • I have updated the list of apps in README.md if (required) with ./arkade install --help

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have tested this on arm, or have added code to prevent deployment

Signed-off-by: Richard Gee <richard@technologee.co.uk>
@@ -3312,38 +3312,56 @@ func Test_DownloadKrew(t *testing.T) {
{
os: "darwin",
arch: arch64bit,
version: "v0.4.2",
url: `https://github.com/kubernetes-sigs/krew/releases/download/v0.4.2/krew-darwin_amd64.tar.gz`,
version: "v0.4.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump the version to current krew release

@@ -3354,7 +3372,7 @@ func Test_DownloadKrew(t *testing.T) {
t.Fatal(err)
}
if got != tc.url {
t.Errorf("want: %s, got: %s", tc.url, got)
t.Errorf("want: %s for %s on %s, got: %s", tc.url, tc.os, tc.arch, got)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added additional detail here because where multiple OS and Arch combinations target a single URI it is challenging to find the failing test case.

@rgee0
Copy link
Contributor Author

rgee0 commented Jan 8, 2023

Url check failing on a unrelated tool:

url_checker_test.go:51: Error with HTTP HEAD for kail, https://github.com/boz/kail/releases/download/v0.16.1/kail_0.16.1_linux_amd64.tar.gz: status code: 404

Actual link is:

https://github.com/boz/kail/releases/download/v0.16.1/kail_v0.16.1_linux_amd64.tar.gz

Note the v prefix in the filename portion.

Signed-off-by: Richard Gee <richard@technologee.co.uk>
@@ -1139,7 +1139,7 @@ https://releases.hashicorp.com/{{.Name}}/{{.Version}}/{{.Name}}_{{.Version}}_{{$
{{$os = "windows"}}
{{- end -}}

{{.Version}}/{{.Name}}_{{.VersionNumber}}_{{$os}}_{{$arch}}.tar.gz`,
{{.Version}}/{{.Name}}_v{{.VersionNumber}}_{{$os}}_{{$arch}}.tar.gz`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this in a later commit after the url checker tests failed in the CI. Kail seem to have changed the asset names from v0.16.0

url: `https://github.com/kubernetes-sigs/krew/releases/download/v0.4.3/krew-darwin_arm64.tar.gz`,
},
{
os: "ming",
Copy link
Owner

Choose a reason for hiding this comment

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

What's this entry for? Windows + MacOS 64-bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I meant to comment here: https://github.com/alexellis/arkade/pull/837/files#diff-6e1468cd6d67c262166efacb61ddf1e9dbb404be9ca1acd3ce0c2c2abc95a741R1031

To the effect that i didn't understand the darwin entry under ming but added the additional arch entry for consistency

version: "v0.4.2",
url: `https://github.com/kubernetes-sigs/krew/releases/download/v0.4.2/krew-darwin_amd64.tar.gz`,
version: "v0.4.3",
url: `https://github.com/kubernetes-sigs/krew/releases/download/v0.4.3/krew-darwin_amd64.tar.gz`,
},
{
os: "darwin",
arch: archARM64,
Copy link
Owner

Choose a reason for hiding this comment

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

I think this test may have been wrong in the first place, since the value should be archDarwinARM64 from uname on MacOS M1

Darwin am1.local 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:29 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T8101 arm64

cc @Jasstkn

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it should be archDarwinARM64.

Signed-off-by: Richard Gee <richard@technologee.co.uk>
Copy link
Owner

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Approved

@alexellis alexellis merged commit aa46e85 into alexellis:master Jan 10, 2023
@rgee0 rgee0 deleted the krew branch January 10, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

krew broken
3 participants