Review: Needs Fixing
Hi, I had another look and added some inline comments.
It would be nice to have the script auto-generate the dates, but this is good
enough for now!
Diff comments:
> diff --git
> a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/update_ubuntu_csv
>
> b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/update_ubuntu_csv
> new file mode 100755
> index 0000000..7427f13
> --- /dev/null
> +++
> b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/update_ubuntu_csv
> @@ -0,0 +1,84 @@
> +#!/bin/bash
> +# shellcheck disable=SC1078,SC1079
> +
> +HELP_STRING='''#==========================================================================================================#
This is not Python :-) Replace ''' with ' and drop the shellcheck disables.
''' is an empty string ('') followed by a '.
> +This script is used to add a new release to
> /usr/share/distro-info/ubuntu.csv on all of the machines in
> +the autopkgtest-cloud environment.
> +
> +Usage (LTS):
> +version,codename,series,created,release,eol,eol-server,eol-esm
> +Example (LTS):
> +./update_ubuntu_csv "24.04 LTS,Noble
> Newt,noble,2023-10-19,2024-04-20,2029-04-20,2029-04-20,2034-04-20"
> +Usage (non-LTS):
> +version,codename,series,created,release,eol
> +Example (non-LTS):
> +./update_ubuntu_csv "24.10,Ostracized
> Octopus,ostracized,2024-04-20,2024-10-19,2025-12-10"
> +
> +In case the user of this script messes up and adds an incorrect line to
> /usr/share/disto-info/ubuntu.csv,
> +please run the following:
> +./update_ubuntu_csv RESTORE
> +which will restore the /usr/share/distro-info/ubuntu.csv file on all the
> machines in the environment to
> +the file on the bastion.
> +
> +The file created on the remote machine is the file on the bastion, appended
> with the user input, not the
> +file from the remote machine
> +#==========================================================================================================#
> +'''
> +
> +set -e
We want this at the very beginning of the script (first command to be executed).
> +
> +if [[ $# -ne 1 ]]; then
> + printf "Number of input arguments wrong, please check them.\n"
> + exit 1
> +fi
> +
> +if [[ "${1}" == "-h" || "${1}" == "--help" ]]; then
> + printf "%s" "${HELP_STRING}"
> + exit 0
> +fi
> +
> +ORIG_CSV=$(cat /usr/share/distro-info/ubuntu.csv)
I don't think we need to read the ubuntu.csv contents into a variable. The file
is not huge, but still I don't find it a good practice, and we're reading it
into a variable only to dump it into a file later in the script...
However let's have this to avoid duplication (writing the path several times in
the script):
ORIG_CSV=/usr/share/distro-info/ubuntu.csv
> +
> +RELEASE_TO_ADD="${1}"
> +RELEASE_ID=$(echo "${RELEASE_TO_ADD}" | cut -d',' -f1)
> +
> +if [[ "${ORIG_CSV}" =~ .*"${RELEASE_ID}".* ]]; then
Assuming $ORIG_CSV is now the path the original csv, we can do this:
if grep -q "^$RELEASE_ID," "$ORIG_CSV"; then
This checks for the RELEASE_ID in the "right" place of the csv (first field).
> + printf "This release already exists. Check state of
> /usr/share/distro-info/ubuntu.csv on the bastion, and try again."
Missing final newline.
> + exit 1
> +fi
> +
> +if [[ "${RELEASE_TO_ADD}" == "RESTORE" ]]; then
> + printf "%s" "${ORIG_CSV}" > "${HOME}"/ubuntu.csv
> +else
> + printf "%s\n%s" "${ORIG_CSV}" "${RELEASE_TO_ADD}" >
> "${HOME}"/ubuntu.csv
> +fi
I don't like that we're dropping a temporary file in $HOME. At the beginning of
the script, around where ORIG_CSV is defined, create a temporary file:
MANGLED_CSV=$(mktemp)
This will have sane/safe location, permissions, etc (see the mktemp manpage).
Then, as we are not reading the orig_csv contents to a variable anymore, we can
do something simpler:
cp "$ORIG_CSV" "$MANGLED_CSV"
if [[ "${RELEASE_TO_ADD}" != "RESTORE" ]]; then
echo "${RELEASE_TO_ADD}" >> "$MANGLED_CSV"
fi
> +
> +juju machines --format=json | jq '.machines' > /tmp/machines.json
> +jq 'keys' /tmp/machines.json > /tmp/machine_keys.json
> +
> +jq -c '.[]' /tmp/machine_keys.json | while read -r i; do
> + MACHINE_ID=$(printf "%s" "${i}" | sed 's/"//g')
> + printf "%s " "${MACHINE_ID}" >> /tmp/machine_nums
> +done
> +
> +ITERATE_ME=$(cat /tmp/machine_nums)
> +rm /tmp/machine_nums /tmp/machines.json /tmp/machine_keys.json
> +
> +for MACHINE in $ITERATE_ME; do
All of this (from `juju machines` on) looks more complex than necessary to me.
We can just:
MACHINES=$(juju machines --format=json | jq -r '.machines | keys[]')
for MACHINE in $MACHINES; do
...
done
After writing this I noticed that Brian already suggested something very
similar.
> + printf
> "========================================================================\n"
> + printf "Checking validity of machine: %s\n" "${MACHINE}"
Do we really need this validity check? Those are machine IDs provided by juju
after all, and now we're trying to validate them using juju again. What case is
this verification going to catch?
> + CHECK_MACHINES=$(juju show-machine "${MACHINE}")
> + if [[ "${CHECK_MACHINES}" =~ .*"machines: {}".* ]]; then
> + printf "Invalid machine %s\n" "${MACHINE}"
> + else
> + printf "Copying file to machine: %s\n" "${MACHINE}"
> + juju scp "${HOME}"/ubuntu.csv "${MACHINE}":/home/ubuntu
Again, let's not use $HOME as a temporary area. Copy $MANGLED_CSV to the same
path on the remote side. Something like
juju scp "$MANGLED_CSV" "$MACHINE:$MANGLED_CSV"
> + juju ssh "${MACHINE}" "sudo bash -c 'mv
> /home/ubuntu/ubuntu.csv /usr/share/distro-info/ubuntu.csv'"
We can't we just `sudo mv`?
> + fi
> +done
> +
> +printf
> "========================================================================\n"
> +printf "All done!\nFYI, this is the last few lines of the file you've copied
> across: \n\n"
> +tail -3 < "${HOME}"/ubuntu.csv
Nit: redirect not needed
> +printf "\n\n"
> +rm "${HOME}"/ubuntu.csv
> \ No newline at end of file
--
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/442546
Your team Canonical's Ubuntu QA is subscribed to branch
autopkgtest-cloud:master.
--
Mailing list: https://launchpad.net/~canonical-ubuntu-qa
Post to : canonical-ubuntu-qa@lists.launchpad.net
Unsubscribe : https://launchpad.net/~canonical-ubuntu-qa
More help : https://help.launchpad.net/ListHelp
