-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
Add AWS CLI to system install
tools
#993
base: master
Are you sure you want to change the base?
Conversation
@Jasstkn would you be able to take a look at the approach? |
sure, will take a look. In the meantime, @VariableExp0rt can I ask you to add a confirmation how you tested the installation via arkade? |
@Jasstkn I tested via |
Hey @Jasstkn, let me know if there is anything else you need from me :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments above and do a rebase!
Overall really nice work, I really like the tool unit test for this too, it is a cool idea!
Thank you for your contribution. It seems that one or more of your commits have a "Signed-off-by" statement with an anonymous email address. The Developer Certificate of Origin (DCO) requires all commits to be signed off by genuine, contactable individuals. Please see our contributing guide.
This will only take a few moments. First, clone your fork and checkout this branch using the git CLI. Next, set up your real name and email address:
Finally, run one of these commands to add the "Signed-off-by" line to your commits. If you only have one commit so far then run: Check that the message has been added properly by running "git log". |
4b85a9f
to
26a1b6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising.
Please don't forget to update the README.md and squash the commits in to single one, we like to keep the git history clean for single tools like this.
Thanks!
|
402274a
to
c1862d9
Compare
Signed-off-by: liam.baker <[email protected]>
3670984
to
35fd063
Compare
@Shikachuu I think I've managed to fix it, sorry about that! Let me know if there are any more comments to address 👍 |
For posterity:
|
@Shikachuu is there anything else I should do here? |
} | ||
|
||
func getAWSCLIVersion(owner, repo string) (string, error) { | ||
url := fmt.Sprintf("https://api.github.com/repos/%s/%s/git/refs/tags", owner, repo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot use the API because it's rate-limited.
I'm curious, why did you add this when we already have code in the project (everywhere) that already determines the version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this is that tags exist that are for all releases, but there are not releases to accompany them. I might be wrong, but I couldn't see specifically anywhere that only looks at the refs/tags (which is all that AWS provide). More info here #993 (comment)
return latest.String(), nil | ||
} | ||
|
||
func runBundledInstaller(osVer string, workDir string, filename string, installPath string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this option is being included?
What format is the AWS CLI in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLI comes in different formats for different platforms, for instance OSx is a .pkg, Linux is a .zip, and similar for Windows. There isn't just a binary.
} | ||
|
||
func installationInstructions(osVer string, workDir string, filename string, installPath string) ([]byte, error) { | ||
t := template.New("Installation Instructions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no binary package for the aws cli?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not, just zip, pkg, or the msi installer for Windows.
Description
This PR attempts to add support for downloading and installing the AWS CLI for supported platforms. This turned out to be a bit more difficult with the special requirements for each platform supported (Linux, MacOS, Windows).
Motivation and Context
Fixes a currently open issue - #941
How Has This Been Tested?
Changes tested with:
If updating or adding a new CLI to
arkade get
, run:Types of changes
Documentation
./arkade get --format markdown
./arkade install --help
Checklist:
git commit -s
(I've added a boolean flag
--run-installer
which defaults to false)