canonical-ubuntu-qa team mailing list archive

[Paride Legovini <mp+442546@xxxxxxxxxxxxxxxxxx>]

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.