Description
Hi,
As I'm not 100% clear on the expected coding style and/or contribution guidelines here, I'm going to raise this as an issue rather than a PR and see what gets accepted/influenced/rejected.
First, a tiny critique: there's a number of ShellCheck SC2155's. I see that this has been ad-hoc disabled globally in at least one other lib file, which seems to me to be a sub-optimal choice. FWIW my take on this rule is to simply use the "everybody's happy" option of just declaring your local vars once on a single line and moving on.
With respect specifically to instance-ssh()
, this:
local instance_details="$(instance-ssh-details $instance_id)"
local instance_id=$(echo $instance_details | awk '{print $1}')
local keyname=$(echo $instance_details | awk '{print $2}')
local private_ip=$(echo $instance_details | awk '{print $3}')
local instance_name=$(echo $instance_details | awk '{print $4}')
local instance_default_user=$(echo $instance_details | awk '{print $5}')
... could potentially be expressed more efficiently like so:
local instance_id keyname private_ip instance_name instance_default_user
read -r instance_id keyname private_ip instance_name instance_default_user < <(instance-ssh-details "$instance_id")
Or perhaps something like this:
local instance_id keyname private_ip instance_name instance_default_user
set -- $(instance-ssh-details "$instance_id")
instance_id="$1"
keyname="$2"
... etc
The other tweak I'd like to suggest is changing this:
-i "${BMA_SSH_DIR:-~/.ssh/}$keyname"
to
-i "${BMA_SSH_DIR:-~/.ssh}/$keyname"
The rationale is that if BMA_SSH_DIR
is missing a trailing slash, this will ensure that one is present. If BMA_SSH_DIR
does have a trailing slash, then a non-fatal double-slash will be present (i.e. /path/to/BMA_SSH_DIR//keyname
) and the connection will still work. Either way, the default behaviour of ~/.ssh/$keyname
remains intact.
With the current code, a BMA_SSH_DIR
without a trailing slash results in e.g. /path/to/BMA_SSH_DIRkeyname
which will obviously fail.
Cheers!