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

Feature: check_or_install talosctl #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TrooperT
Copy link

@TrooperT TrooperT commented Aug 8, 2024

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
    • prepended to $PATH
    • serves to store local-use copies of any downloaded binaries

New soft dependencies:

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
Copy link
Member

@kvaps kvaps Aug 17, 2024

Choose a reason for hiding this comment

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

  1. This adds additional dependency - bash and all dependencies used by i.jpillora.com, which was not required to use this script before.

  2. 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

Copy link
Member

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:

https://github.com/deckhouse/deckhouse/blob/ab6faa3ad220f203117626a15052333bf84154dc/Makefile#L11-L46

Copy link
Author

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 :)

Copy link
Member

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.

talos-bootstrap Outdated Show resolved Hide resolved
Co-authored-by: Andrei Kvapil <[email protected]>
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.

talosctl: This program can only be run on AMD64 processors with v3 microarchitecture support
2 participants