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

Support arkade get with tgz/zip archives #130

Closed
wants to merge 3 commits into from

Conversation

tuananh
Copy link
Contributor

@tuananh tuananh commented Jun 19, 2020

Signed-off-by: Tuan Anh Tran me@tuananh.org

Allow arkade get to download tgz/zip archives

There are a few points I still need clarification.

  • for kubectx, the cli name is same as the tool so I can reuse tool.Name. However, will there be case the cli name is different than the repo name? In that case, should I add a new field BinaryName to the Tool struct and use that to pull the cli from the tgz/zip archive instead?
  • Will there be case there will be multiple cli in the same tgz/zip that need to be pull out?
  • I can only test manually by changing the url for now because kubectx author is still recommending to use bash version.

Description

Motivation and Context

Attempt to partially fix #123

  • I have raised an issue to propose this change (required)

How Has This Been Tested?

Right now, there's no tgz/zip tool so I manually change the url of kubectx to use tgz format (hardcode download url to https://github.com/ahmetb/kubectx/releases/download/v0.9.0/kubectx_v0.9.0_darwin_x86_64.tar.gz and NeedDecompress to true during testing.

image

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)

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 added tests to cover my changes.
  • All new and existing tests passed.
  • I have tested this on arm, or have added code to prevent deployment

Signed-off-by: Tuan Anh Tran <me@tuananh.org>
@derek derek bot added the no-dco label Jun 19, 2020
@derek
Copy link

derek bot commented Jun 19, 2020

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

…ade-get-tgz-zip

Signed-off-by: Tuan Anh Tran <me@tuananh.org>
@tuananh tuananh force-pushed the support-arkade-get-tgz-zip branch from 8adf5fa to d3bbe8d Compare June 19, 2020 03:41
@derek derek bot removed the no-dco label Jun 19, 2020
cmd/get.go Outdated Show resolved Hide resolved
pkg/get/get.go Outdated
@@ -195,7 +196,8 @@ https://storage.googleapis.com/kubernetes-release/release/{{.Version}}/bin/{{$os
Version: "v0.9.0",
URLTemplate: `https://github.com/ahmetb/kubectx/releases/download/{{.Version}}/kubectx`,
Copy link
Owner

Choose a reason for hiding this comment

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

Theoretically this is now wrong as a URL if you are downloading the archive?

How about leaving this one as it is, and adding helm3 to the list of "get" tools instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

helm added
image

cmd/get.go Outdated
"github.com/spf13/cobra"
)

func IsArchive(url string) bool {
return strings.HasSuffix(url, "tar.gz") || strings.HasSuffix(url, "zip")
Copy link
Owner

Choose a reason for hiding this comment

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

We can't support .zip at this time. I would remove that code.

Also IsArchive could be part of the Tool type, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

if err != nil {
t.Fatal(err)
}
want := "https://get.helm.sh/helm-v3.2.4-windows-amd64.zip"
Copy link
Owner

Choose a reason for hiding this comment

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

Is the un-tar code really going to work with a zip?

Copy link
Owner

Choose a reason for hiding this comment

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

My gut feeling is: no. However, there is a Go library that can uncompress zips -> https://golang.org/src/archive/zip/example_test.go

Copy link
Owner

Choose a reason for hiding this comment

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

I may be wrong here,

tar -xvf helm-v3.2.4-windows-amd64.zip 
x windows-amd64/
x windows-amd64/helm.exe
x windows-amd64/README.md
x windows-amd64/LICENSE

Copy link
Owner

Choose a reason for hiding this comment

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

bsdtar can uncompress zip archives, but I would think the Golang tar code cannot. In any case using archive/zip for zips would be better suited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexellis you're right. it doesn't work with zip file. I add Unzip function to deal with that.

Since i don't have a windows machine, I just hardcode a zip url and test . it's able to extract the zip files in the pic below

image

Signed-off-by: Tuan Anh Tran <me@tuananh.org>
@tuananh tuananh force-pushed the support-arkade-get-tgz-zip branch from 482cc38 to 27dbd04 Compare June 19, 2020 17:35
return err
}
reader := bytes.NewReader(buff.Bytes())
zipReader, err := zip.NewReader(reader, size)
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 we can improve the API here - for instance, let's pass in the bytes directly to the method and have it create the zip reader via the zip.NewReader call.

alexellis pushed a commit that referenced this pull request Jun 19, 2020
Introduces support for "get" command to use tgz and zip archives
and adds helm3 option to "get" command.

This commit squashes three commits from PR #130 and closes #130.

Signed-off-by: Tuan Anh Tran <me@tuananh.org>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
alexellis pushed a commit that referenced this pull request Jun 19, 2020
Introduces support for "get" command to use tgz and zip archives
and adds helm3 option to "get" command.

This commit squashes three commits from PR #130 and closes #130.

Signed-off-by: Tuan Anh Tran <me@tuananh.org>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
alexellis pushed a commit that referenced this pull request Jun 19, 2020
Introduces support for "get" command to use tgz and zip archives
and adds helm3 option to "get" command.

This commit squashes three commits from PR #130 and closes #130.

Signed-off-by: Tuan Anh Tran <me@tuananh.org>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
}()

// Closure to address file descriptors issue with all the deferred .Close() methods
extractAndWriteFile := func(f *zip.File) error {
Copy link
Owner

Choose a reason for hiding this comment

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

Did you copy any of this code from stack overflow or another source like a blog post? If so please quote the URL here for crediting the author.

alexellis pushed a commit that referenced this pull request Jun 19, 2020
Introduces support for "get" command to use tgz and zip archives
and adds helm3 option to "get" command.

This commit squashes three commits from PR #130 and closes #130.

Signed-off-by: Tuan Anh Tran <me@tuananh.org>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
@alexellis
Copy link
Owner

See #133

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.

[Tools] - arkade get should be able to cope with tgz/zip and multi-file archives
2 participants