canonical-ubuntu-qa team mailing list archive

[Tim Andersson <mp+471372@xxxxxxxxxxxxxxxxxx>]

Some inline comments, also, commit b2e5f39 is not sufficient to add pre-commit to the LPCI pipeline. Please see:
https://git.launchpad.net/autopkgtest-cloud/tree/.launchpad.yaml

For how to do this.

Diff comments:

> diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
> new file mode 100644
> index 0000000..7bff89d
> --- /dev/null
> +++ b/.pre-commit-config.yaml
> @@ -0,0 +1,27 @@
> +---
> +repos:
> +  - repo: https://github.com/adrienverge/yamllint.git
> +    rev: v1.29.0
> +    hooks:
> +      - id: yamllint
> +        args: [--strict]
> +  - repo: https://github.com/adrienverge/yamllint.git
> +    rev: v1.29.0
> +    hooks:
> +      - id: yamllint
> +        alias: yamllint-jobs
> +        name: yamllint (for jobs.yaml files)
> +        args: [--strict, -c, .yamllint-jobs]
> +  - repo: https://github.com/shellcheck-py/shellcheck-py
> +    rev: v0.9.0.5
> +    hooks:
> +      - id: shellcheck
> +        args: ['-e', 'SC1090', '-e', 'SC1091', '-e', 'SC2001']

Since we used a configfile for yamllint, let's also use a config file for shellcheck. Details can be found in this commit:
https://github.com/koalaman/shellcheck/commit/581bcc3907ab98e919a7dd60566810a928c46b95

I'm not sure if it'll be available in the pre-commit hook, but let's try anyway :)

> +  - repo: local
> +    hooks:
> +      - id: deploy-jobs
> +        name: deploy-jobs
> +        entry: scripts/deploy-jobs.sh
> +        args:
> +          - "test"
> +        language: script
> diff --git a/.yamllint-jobs b/.yamllint-jobs
> new file mode 100644
> index 0000000..45421e4
> --- /dev/null
> +++ b/.yamllint-jobs
> @@ -0,0 +1,12 @@
> +---
> +extends: default
> +
> +ignore:
> +  - "**/*.yaml"
> +  - "!**/*jobs*.yaml"
> +  - "!jobs/macros.yaml"
> +
> +rules:
> +  indentation: disable
> +  line-length: disable
> +  document-start: disable

Is there a specific reason this rule is disabled? The other two make total sense, though I'm not sure with this one.

> \ No newline at end of file
> diff --git a/scripts/flash-device.sh b/scripts/flash-device.sh
> index 1efa0db..9dbbbb8 100755
> --- a/scripts/flash-device.sh
> +++ b/scripts/flash-device.sh
> @@ -74,67 +76,67 @@ echo "Wifi: $WIFI"
>  echo "Fastboot: $FASTBOOT"
>  echo "Recovery: $RECOVERY"
>  QUERY_CMD="ubuntu-device-flash $SERVER $REVARGS query --channel=$CHANNEL --device $DEVICE --show-image"
> -echo $QUERY_CMD
> +echo "$QUERY_CMD"
>  $QUERY_CMD
>  
>  rootcmd () {
> -  adb -s $ANDROID_SERIAL shell "echo $PASSWD | sudo -S $*"
> +  adb -s "$ANDROID_SERIAL" shell "echo $PASSWD | sudo -S $*"
>  }
>  
>  # Handle fastboot mode and fastboot recovery mode
>  if [ x"$FASTBOOT_RECOVERY" != x ]; then
>    echo 'Rebooting to bootloader/fastboot'
> -  adb -s $ANDROID_SERIAL reboot bootloader
> +  adb -s "$ANDROID_SERIAL" reboot bootloader
>    sleep 10
>    echo "Flashing recovery image $FASTBOOT_RECOVERY"
> -  fastboot -s $ANDROID_SERIAL flash recovery $FASTBOOT_RECOVERY
> +  fastboot -s "$ANDROID_SERIAL" flash recovery "$FASTBOOT_RECOVERY"
>    echo "Rebooting into recovery"
> -  fastboot -s $ANDROID_SERIAL boot $FASTBOOT_RECOVERY
> +  fastboot -s "$ANDROID_SERIAL" boot "$FASTBOOT_RECOVERY"
>    sleep 10
>    FASTBOOT=no
> -elif [ x"$FASTBOOT" = xyes ]; then
> +elif [ "$FASTBOOT" = xyes ]; then

x seems to be removed here which may break the if statement? Though there may be a reason for this I am unaware of :)

>    EXTRA="--bootstrap"
>    echo 'Rebooting to bootloader/fastboot'
> -  adb -s $ANDROID_SERIAL reboot bootloader
> +  adb -s "$ANDROID_SERIAL" reboot bootloader
>    sleep 10
>  fi
>  
>  echo 'flashing device'
>  FLASH_CMD="ubuntu-device-flash $SERVER $REVARGS touch --serial=$ANDROID_SERIAL --channel=$CHANNEL --device $DEVICE $DEVMODE $RECOVERY --wipe $EXTRA"
> -echo $FLASH_CMD
> +echo "$FLASH_CMD"
>  $FLASH_CMD
>  
>  echo 'waiting for device to boot'
> -adb -s $ANDROID_SERIAL wait-for-device
> +adb -s "$ANDROID_SERIAL" wait-for-device
>  echo 'waiting a few seconds for boot'
>  sleep 5
>  
>  # setup network; location of config per plars' advice
>  DEFAULT_NETWORK_CONFIG_PATH="/var/lib/jenkins/.ubuntu-ci/wifi.conf"
> -if [ -n NETWORK_CONFIG_PATH ]; then
> +if [ -n "$NETWORK_CONFIG_PATH" ]; then
>    NETWORK_CONFIG_PATH=$DEFAULT_NETWORK_CONFIG_PATH
>  fi
> -phablet-network -n $NETWORK_CONFIG_PATH
> +phablet-network -n "$NETWORK_CONFIG_PATH"
>  
>  # Disable the welcome wizard
> -if [ x"$WIZARD" = x"no" ]; then
> +if [ "$WIZARD" = x"no" ]; then

same here, x is removed

>    echo 'disabling wizard'
> -  phablet-config -s $ANDROID_SERIAL welcome-wizard --disable
> -  adb -s $ANDROID_SERIAL wait-for-device
> +  phablet-config -s "$ANDROID_SERIAL" welcome-wizard --disable
> +  adb -s "$ANDROID_SERIAL" wait-for-device
>    REBOOT_NEEDED=yes
>  fi
>  
>  # Disable the edges intro
> -if [ x"$EDGEINTRO" = x"no" ]; then
> +if [ "$EDGEINTRO" = x"no" ]; then

x also removed here?

>    echo 'disabling edges intro'
> -  phablet-config -s $ANDROID_SERIAL edges-intro --disable
> -  adb -s $ANDROID_SERIAL wait-for-device
> +  phablet-config -s "$ANDROID_SERIAL" edges-intro --disable
> +  adb -s "$ANDROID_SERIAL" wait-for-device
>  fi
>  
> -if [ x"$REBOOT_NEEDED" = x"yes" ]; then
> +if [ "$REBOOT_NEEDED" = x"yes" ]; then

and the same here

>    echo 'rebooting to activate previous changes'
> -  adb -s $ANDROID_SERIAL reboot
> -  adb -s $ANDROID_SERIAL wait-for-device
> +  adb -s "$ANDROID_SERIAL" reboot
> +  adb -s "$ANDROID_SERIAL" wait-for-device
>  fi
>  
>  # wait until our network is ready


-- 
https://code.launchpad.net/~canonical-platform-qa/qa-jenkins-jobs/+git/qa-jenkins-jobs/+merge/471372
Your team Canonical Platform QA Team is requested to review the proposed merge of qa-jenkins-jobs:pre-commit-hooks into qa-jenkins-jobs:master.