-
Notifications
You must be signed in to change notification settings - Fork 15
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
Feature: check_or_install talosctl #15
base: master
Are you sure you want to change the base?
Conversation
pushd $PWD > /dev/null | ||
mkdir -p $HOME/.tbs/bin && cd $HOME/.tbs/bin | ||
# url without ! will attempt to download to $PWD | ||
curl -s "https://i.jpillora.com/${REPO}?as=${PROG}&type=script" | bash |
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.
-
This adds additional dependency -
bash
and all dependencies used byi.jpillora.com
, which was not required to use this script before. -
I have a fear using external services without informing user about this.
curl | bash
is very bad pattern to include into script. But it can be described in a README.md
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.
How about parsing architecture and platform using uname
and preform url from this:
https://github.com/siderolabs/talos/releases/latest/download/talosctl-<platform>-<arch>
Or if version specified:
https://github.com/siderolabs/talos/releases/download/<version>/talosctl-<platform>-<arch>
See approach from this file:
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.
Those are great points.
- I agree the
curl | bash
is a less-than-ideal pattern, as well as an additional dependency along with all the binaries that the generated install script from J.Pillora's installer checks for [although I believe all the checks are for binaries expected on a POSIX-compliant system]. The design was a "quick-fix" I chose w/o considering the extra deps it added
I'll see what I can do to address your concerns by specializing the check_or_install function to our specific use case for talosctl
FWIW, I initially chose to design in this way anticipation of another feature contribution which will allow the user to select the to-be-installed talosctl version from a list as it would need to ensure yq
was available for easily parsing Github API results, BUT I can still accomplish that and narrow dependencies to just the actual binaries that we need: talosctl and yq.
Ill hammer away and get back with you :)
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.
Thank you @TrooperT ❤️
I mean, that I don't against using curl | bash
but it should be described in README.md
insead of placing it into script.
So user will invoke it at their own risk, or they still have the opportunity to install it from their distribution.
From the other side script can check if required binary exists and can be running.
Co-authored-by: Andrei Kvapil <[email protected]>
Fixes #3
Adds a .gitignore for scratch directories at any depth
Add a reusable shell function with parameters to check for a binary, and prompt with corrective action if missing.
New filesystem paths used/created:
$HOME/.tbs/bin
New soft dependencies: