Closed cmoulliard closed 1 year ago
cmoulliard
commented
2 years ago
- DEST_DIR is never used.. was probably meant to be used at line 150 ? note you should probably give people a heads up you will be copying newly downloaded binaries into /usr/local/bin .. and should also document that users of this script will require sudo access.
Do you agree to install software under /usr/local/bin or should we do that under $HOME/.local/bin ? Is $HOME/.local/bin part of the path of Mac or Linux OS ?
BarDweller
commented
2 years ago
- DEST_DIR is never used.. was probably meant to be used at line 150 ? note you should probably give people a heads up you will be copying newly downloaded binaries into /usr/local/bin .. and should also document that users of this script will require sudo access.
Do you agree to install software under /usr/local/bin or should we do that under $HOME/.local/bin ? Is $HOME/.local/bin part of the path of Mac or Linux OS ?
General rule of thumb is /usr/local/bin is for system admins to provide non distribution related binaries to all the users of a system. If the binaries are just for the current user (which is a lot more common for a developer laptop) then usually you prefer to have them in $home/bin or $home/.local/bin (the latter is from a newer convention, as used by pip and some other stuff, I tend to use $home/bin) .. in general this is why this bit needs configuration, see how ./configure allows setting of a prefix to designate where it should place binaries etc..
As for if it's on the path, I'd need to check, for Linux, depends a lot on the distribution, some add both, some add only one or the other, and some add none. I suspect it would change by version.. if in doubt, just query the path env var =) I have no idea about Mac OS =)
cmoulliard
commented
2 years ago $home/bin or $home/.local/bin
Such paths do not exist OOTB on macos machines.
BarDweller
commented
2 years ago Aye, they often don't on linux systems either, but may still be on the path so they work when they are created.. depends on the distribution though, safer in all cases to just parse the path & check if it's there or not.
cmoulliard
commented
2 years ago Tidy up the script into a collection of bash methods, have each one make sure it checks return codes appropriately, and have each method return a return code that's tested by the overall process..
Example ? @BarDweller
cmoulliard
commented
2 years ago
- also, script is very variable in it's use of
;to avoid newlines, why not properly layout the for loop at line 40, and break apart the multi-statement at line 57 ?.
I do not follow you here. Can you propose something ? @BarDweller
cmoulliard
commented
2 years ago
- lines 114-122 are setting env vars to .. themselves?
So this syntax is wrong REGISTRY_OWNER=${REGISTRY_OWNER} and should be : ${REGISTRY_OWNER} or another example when a default value exists : ${REGISTRY_SERVER:="docker.io"} ? @BarDweller
cmoulliard
commented
2 years ago
- line 292 .. I'm guessing here ghcr.io should actually be $REGISTRY_SERVER
No as currently this package(s) repository that I created only exists on ghcr.io ;-)
cmoulliard
commented
2 years ago
- line 305 .. sleep 10s .. this is going to cause problems.. waiting for any fixed amount of time, and then blindly proceeding onward without testing if whatever you waited for is done, is a bad idea.
I fully agree. I will check if a trick exists to continue the installation if the repository has been well installed ...
cmoulliard
commented
2 years ago What do you think about this bash skeleton to create new bash script ? do we need some additional controls to check using trap errors, etc or enable set -u or set +u?@BarDweller
Todo
Review the remarks of Ozzy -->
General comments...
As an install script, this is a dangerous one. It will only work if everything goes well. It expects to be run with the ability to execute sudo commands, but the script doesn't tell the user what it will do to their system. In some cases (like yum executions) the user will be prompted if they want to make those changes.. in others.. (like the sudo install of k9s, or the curl/untar install of krew, helm, pivnet) the user will have no idea what happened. As a user I'm generally pretty wary of scripts that ask for root access, that mess with my files, and don't tell me what they did.
All the commands are just executed sequentially, the return codes from each command are never tested. For an quick hacky script, that's fine, but for an install script, you want a bit more bullet proofing, to ensure that subsequent steps don't go head if prereqs failed, and should even look at what cleanup needs to be performed if the whole process couldn't complete. After all, if it failed (for example at a wget or curl due to a network issue), there's no way for the user to know what they have to undo, before having their system back in a state where install.sh may be able to run again. Tidy up the script into a collection of bash methods, have each one make sure it checks return codes appropriately, and have each method return a return code that's tested by the overall process..
This script installs many commands directly into /usr/local/bin .. it doesn't check if those commands are already there, at a different version, or give the user a choice of a different location, it just obliterates whatever is there, making sure to use 'sudo' to ensure that happens. Do the commands have to be in /usr/local/bin ? look at how krew installed its binaries without sudo, to a non privileged directory that it asks you to add to the path. That approach is far, far, safer.
Specific comments...
in the defining colors block, put NC at top or the bottom, it's a not a color, and having it sorted into the mix is a bit odd
'repeat_char' method would probably be better called 'generate_eyecatcher'
script would look nicer if the indenting was consistent (eg, see lines 39/40 and line 44/45), probably because tabs have been used.. stick to spaces throughout =)
also, script is very variable in it's use of
;to avoid newlines, why not properly layout the for loop at line 40, and break apart the multi-statement at line 57 ?.not sure why log msg outputs a newline, but log line doesn't.. and since they pretty much do the same thing, maybe pull that out into it's own method, and have each invoke that to retrieve the common part.
DEST_DIR is never used.. was probably meant to be used at line 150 ? note you should probably give people a heads up you will be copying newly downloaded binaries into /usr/local/bin .. and should also document that users of this script will require sudo access.
lines 114-122 are setting env vars to .. themselves?
after saying you 'detect' ubuntu & debian & darwin back in the check_distro command, lines 142-146 attempt to always install a bunch of packages using yum (it tests if it IS fedora, and uses yum to do stuff, but if it wasn't fedora, it still uses yum to do stuff.. ??!!)
more yum usage on line 148
should probably consider how much effort it's going to be for someone to undo the effects of this script if they wanted to.. with so many yum packages installed, and files being copied into /usr/local/bin etc.. this has significant effect on the users distribution.. extreme care required.
the install kubectl crew tool (L152-161) appears to have it's own entire little recreation of what check_distro was doing?? This section might actually support arm.. which is a surprise, because on line 149 we just force downloaded Linux x86/64 binaries, which definitely won't work on arm. Which is correct? is this script intended for multi-arch/multi-distro? or just Linux x86/64 ?
lines 171-172 .. this is not nice.. inside the .bash_aliases script you are modifying the users path to add a new location at the front of their path .. this is almost underhanded, as the user may fairly expect that script to contribute aliases, not alter their path in this manner.
why are the vars defined on lines 211/212/220-225 not all defined together with the ones back at lines 129 ? (same question for 246/247, 262, 267-269 etc). Why are they even vars if they are just defined in the middle of the script and only used once.. if they are the sort of thing that need configuring in the future, and you are trying to define things that future users of install.sh need to edit, then make them all live in a nice config section, with comments explaining what they are, and why they are there.
patch_kapp_configmap will only set $configMap IF a ca was set, but then 241 will always drive kubectl with the $configMap env var (passing an empty string as the --patch arg) .. this feels odd? was it intentional ?
line 292 .. I'm guessing here ghcr.io should actually be $REGISTRY_SERVER
line 305 .. sleep 10s .. this is going to cause problems.. waiting for any fixed amount of time, and then blindly proceeding onward without testing if whatever you waited for is done, is a bad idea.
why is there a '}' on the last line?