From b6a0f9691398424541264f5d81fbeb090bbd48af Mon Sep 17 00:00:00 2001 From: Jan Dubois Date: Fri, 7 Jul 2023 12:33:51 -0700 Subject: [PATCH 1/4] Create deployment profile library for BATS Signed-off-by: Jan Dubois --- bats/tests/containers/factory-reset.bats | 16 -- bats/tests/helpers/defaults.bash | 19 ++ bats/tests/helpers/load.bash | 13 +- bats/tests/helpers/profile.bash | 338 +++++++++++++++++++++++ bats/tests/helpers/utils.bash | 81 +++++- bats/tests/helpers/vm.bash | 26 +- bats/tests/profile/sample.bats | 100 +++++++ 7 files changed, 551 insertions(+), 42 deletions(-) create mode 100644 bats/tests/helpers/profile.bash create mode 100644 bats/tests/profile/sample.bats diff --git a/bats/tests/containers/factory-reset.bats b/bats/tests/containers/factory-reset.bats index bda9208123c..050ff9827d0 100644 --- a/bats/tests/containers/factory-reset.bats +++ b/bats/tests/containers/factory-reset.bats @@ -1,6 +1,4 @@ load '../helpers/load' -assert=assert -refute=refute @test 'factory reset' { factory_reset @@ -156,20 +154,6 @@ rdctl_factory_reset() { fi } -refute_failure() { - assert_success -} - -refute_not_exists() { - assert_exists "$@" -} - -before() { - assert=refute - refute=assert - "$@" -} - check_directories() { # Check if all expected directories are created after starting application/ are deleted after a factory reset delete_dir=("$PATH_APP_HOME" "$PATH_CONFIG") diff --git a/bats/tests/helpers/defaults.bash b/bats/tests/helpers/defaults.bash index 87fdf87c9bb..259d23de236 100644 --- a/bats/tests/helpers/defaults.bash +++ b/bats/tests/helpers/defaults.bash @@ -55,6 +55,13 @@ using_ghcr_images() { is_true "$RD_USE_GHCR_IMAGES" } +######################################################################## +: "${RD_DELETE_PROFILES:=true}" + +deleting_profiles() { + is_true "$RD_DELETE_PROFILES" +} + ######################################################################## : "${RD_USE_IMAGE_ALLOW_LIST:=false}" @@ -62,6 +69,14 @@ using_image_allow_list() { is_true "$RD_USE_IMAGE_ALLOW_LIST" } +######################################################################## +# RD_USE_PROFILE is for internal use. It uses a profile instead of +# settings.json to set initial values for WSL integrations and allowed +# images list because when settings.json exists the default profile is +# ignored. + +: "${RD_USE_PROFILE:=false}" + ######################################################################## : "${RD_USE_VZ_EMULATION:=false}" @@ -126,6 +141,10 @@ validate_enum RD_9P_PROTOCOL_VERSION 9p2000 9p2000.u 9p2000.L validate_enum RD_9P_SECURITY_MODEL passthrough mapped-xattr mapped-file none +######################################################################## +# Use RD_PROTECTED_DOT in profile settings for WSL distro names +: "${RD_PROTECTED_DOT:=ยท}" + ######################################################################## # RD_LOCATION specifies the location where Rancher Desktop is installed # system: default system-wide install location shared for all users diff --git a/bats/tests/helpers/load.bash b/bats/tests/helpers/load.bash index 36d41c743ea..dbef0774d2c 100644 --- a/bats/tests/helpers/load.bash +++ b/bats/tests/helpers/load.bash @@ -47,6 +47,9 @@ source "$PATH_BATS_HELPERS/paths.bash" # and PATH_* variables from paths.bash source "$PATH_BATS_HELPERS/commands.bash" +# profile.bash uses is_xxx() from os.bash +source "$PATH_BATS_HELPERS/profile.bash" + # vm.bash uses various PATH_* variables from paths.bash, # rdctl from commands.bash, and jq_output from utils.bash source "$PATH_BATS_HELPERS/vm.bash" @@ -74,14 +77,14 @@ setup_file() { } teardown_file() { - call_local_function - capture_logs - # On Linux & Windows if we don't shutdown Rancher Desktop bats test don't terminate + # On Linux & Windows if we don't shutdown Rancher Desktop bats tests don't terminate if is_linux || is_windows; then run rdctl shutdown fi + + call_local_function } setup() { @@ -95,9 +98,9 @@ setup() { } teardown() { - call_local_function - if [ -z "$BATS_TEST_SKIPPED" ] && [ -z "$BATS_TEST_COMPLETED" ]; then take_screenshot fi + + call_local_function } diff --git a/bats/tests/helpers/profile.bash b/bats/tests/helpers/profile.bash new file mode 100644 index 00000000000..f4b79fa6432 --- /dev/null +++ b/bats/tests/helpers/profile.bash @@ -0,0 +1,338 @@ +case $OS in +darwin) + PROFILE_SYSTEM_DEFAULTS=/Library/Preferences/io.rancherdesktop.profile.defaults.plist + PROFILE_SYSTEM_LOCKED=/Library/Preferences/io.rancherdesktop.profile.locked.plist + PROFILE_USER_DEFAULTS="${HOME}${PROFILE_SYSTEM_DEFAULTS}" + PROFILE_USER_LOCKED="${HOME}${PROFILE_SYSTEM_LOCKED}" + ;; +linux) + PROFILE_SYSTEM_DEFAULTS=/etc/rancher-desktop/defaults.json + PROFILE_SYSTEM_LOCKED=/etc/rancher-desktop/locked.json + PROFILE_USER_DEFAULTS="${HOME}/.config/rancher-desktop.defaults.json" + PROFILE_USER_LOCKED="${HOME}/.config/rancher-desktop.locked.json" + ;; +windows) + PROFILE='Software\Policies\Rancher Desktop' + PROFILE_SYSTEM_DEFAULTS="HKLM\\${PROFILE}\\Defaults" + PROFILE_SYSTEM_LOCKED="HKLM\\${PROFILE}\\Locked" + PROFILE_USER_DEFAULTS="HKCU\\${PROFILE}\\Defaults" + PROFILE_USER_LOCKED="HKCU\\${PROFILE}\\Locked" + + # The legacy profiles (for both system and user) are supported for backward + # compatibility with Rancher Desktop 1.8.x. For BATS purposes the legacy + # user profiles have the advantage of being writable without admin rights. + PROFILE='Software\Rancher Desktop\Profile' + PROFILE_SYSTEM_LEGACY_DEFAULTS="HKLM\\${PROFILE}\\Defaults" + PROFILE_SYSTEM_LEGACY_LOCKED="HKLM\\${PROFILE}\\Locked" + PROFILE_USER_LEGACY_DEFAULTS="HKCU\\${PROFILE}\\Defaults" + PROFILE_USER_LEGACY_LOCKED="HKCU\\${PROFILE}\\Locked" + ;; +esac + +PROFILE_SYSTEM=system +PROFILE_SYSTEM_LEGACY=system-legacy +PROFILE_USER=user +PROFILE_USER_LEGACY=user-legacy + +PROFILE_DEFAULTS=defaults +PROFILE_LOCKED=locked + +# Default location is a writable user location +if is_windows; then + PROFILE_LOCATION=$PROFILE_USER_LEGACY +else + PROFILE_LOCATION=$PROFILE_USER +fi +PROFILE_TYPE=$PROFILE_DEFAULTS + +# profile_location is a registry key on Windows, or a filename on macOS and Linux. +profile_location() { + local profile=$(to_upper "profile_${PROFILE_LOCATION}_${PROFILE_TYPE}" | tr - _) + echo "${!profile}" +} + +# Execute command for each profile +foreach_profile() { + local locations=("$PROFILE_SYSTEM" "$PROFILE_USER") + if is_windows; then + locations+=("$PROFILE_SYSTEM_LEGACY" "$PROFILE_USER_LEGACY") + fi + + local PROFILE_LOCATION PROFILE_TYPE + for PROFILE_LOCATION in "${locations[@]}"; do + for PROFILE_TYPE in "$PROFILE_DEFAULTS" "$PROFILE_LOCKED"; do + "$@" + done + done +} + +# Check if profile exists +profile_exists() { + case $OS in + darwin | linux) + [[ -f $(profile_location) ]] + ;; + windows) + profile_reg query &>/dev/null + ;; + esac +} + +# Create empty profile +create_profile() { + case $OS in + darwin) + profile_plutil -create xml1 + ;; + linux) + local filename=$(profile_location) + profile_sudo mkdir -p "$(dirname "$filename")" + echo "{}" | profile_cat "$filename" + ;; + windows) + # Make sure any old profile data at this location is removed + run profile_reg delete "." + ;; + esac +} + +# Completely remove the profile. Ignores error if profile doesn't exist +delete_profile() { + if deleting_profiles; then + case $OS in + darwin | linux) + run profile_sudo rm -f "$(profile_location)" + ;; + windows) + run profile_reg delete "." + ;; + esac + fi +} + +# Export/copy profile to a directory +export_profile() { + local dir=$1 + if profile_exists; then + local export="${dir}/profile.${PROFILE_LOCATION}.${PROFILE_TYPE}" + case $OS in + darwin | linux) + local filename=$(profile_location) + # Keep .plist or .json file extension + cp "$filename" "${export}.${filename##*.}" + ;; + windows) + profile_reg export "${export}.reg" /y + ;; + esac + fi +} + +# Add boolean setting; value must be "true" or "false" +add_profile_bool() { + local setting=$1 + local value=$2 + + case $OS in + darwin) + profile_plutil -replace "$setting" -bool "$value" + ;; + linux) + profile_jq ".${setting} = ${value}" + ;; + windows) + if [[ $value == true ]]; then + profile_reg add "$setting" /t REG_DWORD /d 1 + else + profile_reg add "$setting" /t REG_DWORD /d 0 + fi + ;; + esac +} + +add_profile_int() { + local setting=$1 + local value=$2 + + case $OS in + darwin) + profile_plutil -replace "$setting" -integer "$value" + ;; + linux) + profile_jq ".${setting} = ${value}" + ;; + windows) + profile_reg add "$setting" /t REG_DWORD /d "$value" + ;; + esac +} + +add_profile_string() { + local setting=$1 + local value=$2 + + case $OS in + darwin) + profile_plutil -replace "$setting" -string "$value" + ;; + linux) + profile_jq ".${setting} = $(json_string "$value")" + ;; + windows) + profile_reg add "$setting" /t REG_SZ /d "$value" + ;; + esac +} + +add_profile_list() { + local setting=$1 + shift + + local elem + case $OS in + darwin) + profile_plutil -replace "$setting" -array + for elem in "$@"; do + profile_plutil -insert "$setting" -string "$elem" -append + done + ;; + linux) + profile_jq ".${setting} = []" + for elem in "$@"; do + profile_jq ".${setting} += [$(json_string "$elem")]" + done + ;; + windows) + # TODO: what happens when the values contain whitespace or quote characters? + profile_reg add "$setting" /t REG_MULTI_SZ /d "$(join_map '\0' echo "$@")" + ;; + esac +} + +# Remove a key or named value from the profile. +# Use a trailing dot to specify that the setting points to a key, e.g. "foo.bar.". +# It only makes a difference on Windows but will work on all platforms. +remove_profile_entry() { + local setting=$1 + + case $OS in + darwin) + run profile_plutil -remove "${setting%.}" + ;; + linux) + run profile_jq "del(.${setting%.})" + ;; + windows) + run profile_reg delete "$setting" + ;; + esac +} + +################################################################################ +# functions defined below this line are implementation detail and should not +# be called directly from any tests. +################################################################################ + +# Returns number of setting segments (separated by dots), e.g. foo.bar.baz returns 3 +count_setting_segments() { + echo "${1//./$'\n'}" | wc -l +} + +# Usage: profile_jq $expr +# +# Applies $expr against the profile and updates it in-places. +profile_jq() { + local expr=$1 + local filename=$(profile_location) + # Need to use a temp file to avoid truncating the file before it has been read. + jq "$expr" "$filename" | profile_cat "${filename}.tmp" + profile_sudo mv "${filename}.tmp" "$filename" +} + +# Usage: profile_plutil $action $options +# +# For -insert|-replace|-remove actions it will make sure all higher level +# dictionaries are created first because plutil doesn't do it by itself. +profile_plutil() { + local action=$1 + + # Make sure all the dictionaries for the setting path exist + if [[ $action =~ ^-insert|-replace|-remove$ ]]; then + local setting=$2 + local count=$(count_setting_segments "$setting") + if ((count > 1)); then + local index + for index in $(seq $((count - 1))); do + local keypath=$(echo "$setting" | cut -d . -f 1-"$index") + # Ignore error if dictionary already exists + run profile_sudo plutil -insert "$keypath" -dictionary "$(profile_location)" + done + fi + fi + + profile_sudo plutil "$@" "$(profile_location)" +} + +# Usage: profile_reg $action $options +# or: profile_reg add|delete $setting $options +# +# Determines the $reg_key from both the profile_location() and the $setting. +# Setting `foo.bar.baz` means `foo\bar` is the reg_subkey, and `baz` is the value name. +# +# Special case `foo.bar.` is used only for "delete" action and specifies `foo\bar` +# as the subkey to be deleted (including all values under the key). +profile_reg() { + local action=$1 + shift + + local reg_key=$(profile_location) + if [[ $action =~ ^add|delete$ ]]; then + local setting=$1 + shift + + local count=$(count_setting_segments "$setting") + if ((count > 1)); then + local reg_subkey=$(echo "$setting" | cut -d . -f 1-"$((count - 1))") + # reg_key uses backslashes instead of dot separators + reg_key="${reg_key}\\${reg_subkey//./\\}" + fi + + local reg_value_name=$(echo "$setting" | cut -d . -f "$count") + # reg_value_name may be empty when deleting a registry key instead of a named value + if [[ -n $reg_value_name ]]; then + # turn protected dots back into regular dots again + set - /v "${reg_value_name//$RD_PROTECTED_DOT/.}" "$@" + fi + + # Delete entries (and overwrite existing ones) without prompt + set - "$@" /f + fi + + reg.exe "$action" "$reg_key" "$@" +} + +profile_sudo() { + # TODO How can we make this work on Windows? + if [[ $PROFILE_LOCATION == system ]]; then + sudo -n "$@" + else + "$@" + fi +} + +profile_cat() { + profile_sudo tee "$1" >/dev/null +} + +ensure_profile_is_deleted() { + delete_profile + if profile_exists; then + fatal "Cannot delete $(profile_location)" + fi +} + +# Only run this once per test file. It cannot be part of setup_file() because +# we want to be able to call fatal() and skip the rest of the tests. +if [[ -z ${BATS_SUITE_TEST_NUMBER-} ]] && deleting_profiles; then + foreach_profile ensure_profile_is_deleted +fi diff --git a/bats/tests/helpers/utils.bash b/bats/tests/helpers/utils.bash index 296cd357cf3..5435488d10d 100644 --- a/bats/tests/helpers/utils.bash +++ b/bats/tests/helpers/utils.bash @@ -1,6 +1,14 @@ +to_lower() { + echo "$@" | tr '[:upper:]' '[:lower:]' +} + +to_upper() { + echo "$@" | tr '[:lower:]' '[:upper:]' +} + is_true() { # case-insensitive check; false values: '', '0', 'no', and 'false' - local value="$(echo "$1" | tr '[:upper:]' '[:lower:]')" + local value=$(to_lower "$1") if [[ $value =~ ^(0|no|false)?$ ]]; then false else @@ -39,6 +47,57 @@ assert_nothing() { true } +######################################################################## + +assert=assert +refute=refute + +before() { + local assert=refute + local refute=assert + "$@" +} + +refute_success() { + assert_failure +} + +refute_failure() { + assert_success +} + +refute_not_exists() { + assert_exists "$@" +} + +######################################################################## + +# Convert raw string into properly quoted JSON string +json_string() { + echo -n "$1" | jq -Rr @json +} + +# Join list elements by separator after converting them via the mapping function +# Examples: +# join_map "/" echo usr local bin => usr/local/bin +# join_map ", " json_string a b\ c\"d\\e f => "a", "b c\"d\\e", "f" +join_map() { + local sep=$1 + local map=$2 + shift 2 + + local elem result + for elem in "$@"; do + elem=$(eval "$map" '"$elem"') + if [[ -z $result ]]; then + result=$elem + else + result="${result}${sep}${elem}" + fi + done + echo "$result" +} + jq_output() { jq -r "$@" <<<"${output}" } @@ -102,14 +161,14 @@ try() { return "$status" } -image_without_tag() { +image_without_tag_as_json_string() { local image=$1 # If the tag looks like a port number and follows something that looks # like a domain name, then don't strip the tag (e.g. foo.io:5000). if [[ ${image##*:} =~ ^[0-9]+$ && ${image%:*} =~ \.[a-z]+$ ]]; then - echo "$image" + json_string "$image" else - echo "${image%:*}" + json_string "${image%:*}" fi } @@ -117,16 +176,7 @@ update_allowed_patterns() { local enabled=$1 shift - local patterns="" - local image - for image in "$@"; do - image=$(image_without_tag "$image") - if [ -z "$patterns" ]; then - patterns="\"${image}\"" - else - patterns="$patterns, \"${image}\"" - fi - done + local patterns=$(join_map ", " image_without_tag_as_json_string "$@") # TODO TODO TODO # Once https://github.com/rancher-sandbox/rancher-desktop/issues/4939 has been @@ -172,6 +222,9 @@ capture_logs() { mkdir -p "$logdir" cp -LR "$PATH_LOGS/" "$logdir" echo "${BATS_TEST_DESCRIPTION:-teardown}" >"$logdir/test_description" + # Capture settings.json + cp "$PATH_CONFIG_FILE" "$logdir" + foreach_profile export_profile "$logdir" fi } diff --git a/bats/tests/helpers/vm.bash b/bats/tests/helpers/vm.bash index bcc31ff1840..cf80a37d763 100644 --- a/bats/tests/helpers/vm.bash +++ b/bats/tests/helpers/vm.bash @@ -67,9 +67,11 @@ start_container_engine() { local args=( --application.debug --application.updater.enabled=false - --container-engine.name="$RD_CONTAINER_ENGINE" --kubernetes.enabled=false ) + if [ -n "$RD_CONTAINER_ENGINE" ]; then + args+=(--container-engine.name="$RD_CONTAINER_ENGINE") + fi if is_unix; then args+=( --application.admin-access=false @@ -99,16 +101,25 @@ start_container_engine() { # TODO containerEngine.allowedImages.patterns and WSL.integrations # TODO cannot be set from the commandline yet image_allow_list="$(bool using_image_allow_list)" - wsl_integrations="{}" registry="docker.io" if using_ghcr_images; then registry="ghcr.io" fi - if is_windows; then - wsl_integrations="{\"$WSL_DISTRO_NAME\":true}" - fi - mkdir -p "$PATH_CONFIG" - cat <"$PATH_CONFIG_FILE" + if is_true "${RD_USE_PROFILE-}"; then + if is_windows; then + # Translate any dots in the distro name into $RD_PROTECTED_DOT (e.g. "Ubuntu-22.04") + # so that they are not treated as setting separator characters. + add_profile_bool "WSL.integrations.${WSL_DISTRO_NAME//./$RD_PROTECTED_DOT}" true + fi + add_profile_bool containerEngine.allowedImages.enabled "$image_allow_list" + add_profile_list containerEngine.allowedImages.patterns "$registry" + else + wsl_integrations="{}" + if is_windows; then + wsl_integrations="{\"$WSL_DISTRO_NAME\":true}" + fi + mkdir -p "$PATH_CONFIG" + cat <"$PATH_CONFIG_FILE" { "version": 7, "WSL": { "integrations": $wsl_integrations }, @@ -120,6 +131,7 @@ start_container_engine() { } } EOF + fi if using_npm_run_dev; then # translate args back into the internal API format diff --git a/bats/tests/profile/sample.bats b/bats/tests/profile/sample.bats new file mode 100644 index 00000000000..5938b5ef5b0 --- /dev/null +++ b/bats/tests/profile/sample.bats @@ -0,0 +1,100 @@ +load '../helpers/load' + +local_setup() { + # profile settings should be the opposite of the default config + if using_docker; then + PROFILE_CONTAINER_ENGINE=containerd + else + PROFILE_CONTAINER_ENGINE=moby + fi + PROFILE_START_IN_BACKGROUND=true + RD_USE_PROFILE=true +} + +local_teardown_file() { + foreach_profile delete_profile +} + +rdctl_api_settings() { + run rdctl api /settings + echo "$output" + assert_success || return + refute_output undefined +} + +start_app() { + # Store WSL integration and allowed images list in locked profile instead of settings.json + PROFILE_TYPE=$PROFILE_LOCKED + start_container_engine + try --max 20 --delay 5 rdctl_api_settings + + RD_CONTAINER_ENGINE=$(jq_output .containerEngine.name) + wait_for_container_engine +} + +verify_settings() { + PROFILE_TYPE=$PROFILE_LOCKED + run profile_exists + "${assert}_success" + + PROFILE_TYPE=$PROFILE_DEFAULTS + run profile_exists + "${assert}_success" + + run get_setting .containerEngine.name + "${assert}_output" "$PROFILE_CONTAINER_ENGINE" + + run get_setting .application.startInBackground + "${assert}_output" "$PROFILE_START_IN_BACKGROUND" +} + +@test 'initial factory reset' { + factory_reset +} + +@test 'start up without profile' { + RD_USE_PROFILE=false + start_app +} + +@test 'verify default settings' { + before verify_settings +} + +@test 'factory reset' { + factory_reset +} + +@test 'create profile' { + PROFILE_TYPE=$PROFILE_LOCKED + create_profile + add_profile_string containerEngine.name "$PROFILE_CONTAINER_ENGINE" + + PROFILE_TYPE=$PROFILE_DEFAULTS + create_profile + add_profile_bool application.startInBackground "$PROFILE_START_IN_BACKGROUND" +} + +@test 'start app with new profile' { + RD_CONTAINER_ENGINE="" + start_app +} + +@test 'verify profile settings' { + verify_settings +} + +@test 'change defaults profile setting' { + rdctl set --application.start-in-background=false +} + +@test 'restart app' { + rdctl shutdown + RD_CONTAINER_ENGINE="" + start_app +} + +@test 'verify that defaults settings are not applied again' { + PROFILE_START_IN_BACKGROUND=false + verify_settings +} From 24cf6bd1262b86f54be6471bd1821e7b8f993e2f Mon Sep 17 00:00:00 2001 From: Jan Dubois Date: Mon, 10 Jul 2023 13:48:34 -0700 Subject: [PATCH 2/4] BATS fixes for Windows Signed-off-by: Jan Dubois --- bats/tests/helpers/commands.bash | 9 --------- bats/tests/helpers/utils.bash | 2 +- bats/tests/helpers/vm.bash | 2 +- bats/tests/registry/creds.bats | 4 ++-- 4 files changed, 4 insertions(+), 13 deletions(-) diff --git a/bats/tests/helpers/commands.bash b/bats/tests/helpers/commands.bash index e869e0e263e..5d16da6a967 100644 --- a/bats/tests/helpers/commands.bash +++ b/bats/tests/helpers/commands.bash @@ -14,12 +14,6 @@ else CONTAINER_ENGINE_SERVICE=docker fi -if is_unix; then - RC_SERVICE=rc-service -elif is_windows; then - RC_SERVICE=wsl-service -fi - if is_macos; then CRED_HELPER="$PATH_RESOURCES/$PLATFORM/bin/docker-credential-osxkeychain" elif is_linux; then @@ -86,6 +80,3 @@ rdshell() { rdsudo() { rdshell sudo "$@" } -rc_service() { - rdsudo "$RC_SERVICE" "$@" -} diff --git a/bats/tests/helpers/utils.bash b/bats/tests/helpers/utils.bash index 5435488d10d..7e7daf4bf97 100644 --- a/bats/tests/helpers/utils.bash +++ b/bats/tests/helpers/utils.bash @@ -89,7 +89,7 @@ join_map() { local elem result for elem in "$@"; do elem=$(eval "$map" '"$elem"') - if [[ -z $result ]]; then + if [[ -z ${result:-} ]]; then result=$elem else result="${result}${sep}${elem}" diff --git a/bats/tests/helpers/vm.bash b/bats/tests/helpers/vm.bash index cf80a37d763..4f7608ad45c 100644 --- a/bats/tests/helpers/vm.bash +++ b/bats/tests/helpers/vm.bash @@ -191,7 +191,7 @@ assert_service_status() { local service_name=$1 local expect=$2 - run rc_service "$service_name" status + run rdsudo rc-service "$service_name" status # Some services (e.g. k3s) report non-zero status when not running if [[ $expect == started ]]; then assert_success || return diff --git a/bats/tests/registry/creds.bats b/bats/tests/registry/creds.bats index 5a58bf86601..9a93de9eee1 100644 --- a/bats/tests/registry/creds.bats +++ b/bats/tests/registry/creds.bats @@ -157,8 +157,8 @@ verify_default_credStore() { @test 'restart container engine to refresh certs' { skip_for_insecure_registry - rc_service "$CONTAINER_ENGINE_SERVICE" restart - rc_service --ifstarted openresty restart + rdsudo rc-service "$CONTAINER_ENGINE_SERVICE" restart + rdsudo rc-service --ifstarted openresty restart wait_for_container_engine # when Moby is stopped, the containers are stopped as well if using_docker; then From e9e68718d73430ced6805a3d62161498300431c3 Mon Sep 17 00:00:00 2001 From: Jan Dubois Date: Mon, 10 Jul 2023 14:11:35 -0700 Subject: [PATCH 3/4] Don't ignore shellcheck SC2155 "Declare and assign separately to avoid masking return values" Instead of local foo=$(bar) we need to write local foo foo=$(bar) to make sure we throw an error when `bar` returns a non-zero status Signed-off-by: Jan Dubois --- bats/Makefile | 3 +-- bats/tests/extensions/containers.bats | 3 ++- bats/tests/extensions/install.bats | 5 ++-- bats/tests/helpers/load.bash | 3 ++- bats/tests/helpers/profile.bash | 30 ++++++++++++++++-------- bats/tests/helpers/utils.bash | 12 ++++++---- bats/tests/helpers/vm.bash | 6 +++-- bats/tests/k8s/helm-install-rancher.bats | 3 ++- bats/tests/preferences/verify-paths.bats | 3 ++- bats/tests/registry/creds.bats | 3 ++- 10 files changed, 46 insertions(+), 25 deletions(-) diff --git a/bats/Makefile b/bats/Makefile index d847947639a..ef3851c9d0d 100644 --- a/bats/Makefile +++ b/bats/Makefile @@ -9,9 +9,8 @@ containers: # https://www.shellcheck.net/wiki/SC1091 -- Not following: xxx was not specified as input (see shellcheck -x) # https://www.shellcheck.net/wiki/SC2034 -- xxx appears unused. Verify use (or export if used externally) # https://www.shellcheck.net/wiki/SC2154 -- xxx is referenced but not assigned -# https://www.shellcheck.net/wiki/SC2155 -- Declare and assign separately to avoid masking return values -SC_EXCLUDES ?= SC1091,SC2034,SC2154,SC2155 +SC_EXCLUDES ?= SC1091,SC2034,SC2154 .PHONY: lint lint: diff --git a/bats/tests/extensions/containers.bats b/bats/tests/extensions/containers.bats index d0afcd10934..d1612d97204 100644 --- a/bats/tests/extensions/containers.bats +++ b/bats/tests/extensions/containers.bats @@ -97,7 +97,8 @@ encoded_id() { # variant } @test 'compose - with a long name' { - local name="$(id vm-compose)-with-an-unusually-long-name-yes-it-is-very-long" + local name + name="$(id vm-compose)-with-an-unusually-long-name-yes-it-is-very-long" ctrctl tag "$(id vm-compose)" "$name" rdctl extension install "$name" diff --git a/bats/tests/extensions/install.bats b/bats/tests/extensions/install.bats index 1cfef7e0b1b..3c1e769ff51 100644 --- a/bats/tests/extensions/install.bats +++ b/bats/tests/extensions/install.bats @@ -17,8 +17,9 @@ assert_file_contents_equal() { # $have $want assert_file_exist "$have" assert_file_exist "$want" - local have_hash="$(md5sum "$have" | cut -d ' ' -f 1)" - local want_hash="$(md5sum "$want" | cut -d ' ' -f 1)" + local have_hash want_hash + have_hash="$(md5sum "$have" | cut -d ' ' -f 1)" + want_hash="$(md5sum "$want" | cut -d ' ' -f 1)" if [ "$have_hash" != "$want_hash" ]; then printf "expected : %s (%s)\nactual : %s (%s)" \ "$want" "$want_hash" "$have" "$have_hash" | diff --git a/bats/tests/helpers/load.bash b/bats/tests/helpers/load.bash index dbef0774d2c..da13706b14f 100644 --- a/bats/tests/helpers/load.bash +++ b/bats/tests/helpers/load.bash @@ -62,7 +62,8 @@ export PATH="$PATH_BATS_ROOT/bin/${OS/windows/linux}:$PATH" # If called from foo() this function will call local_foo() if it exist. call_local_function() { - local func="local_$(calling_function)" + local func + func="local_$(calling_function)" if [ "$(type -t "$func")" = "function" ]; then eval "$func" fi diff --git a/bats/tests/helpers/profile.bash b/bats/tests/helpers/profile.bash index f4b79fa6432..8518a33653e 100644 --- a/bats/tests/helpers/profile.bash +++ b/bats/tests/helpers/profile.bash @@ -47,7 +47,8 @@ PROFILE_TYPE=$PROFILE_DEFAULTS # profile_location is a registry key on Windows, or a filename on macOS and Linux. profile_location() { - local profile=$(to_upper "profile_${PROFILE_LOCATION}_${PROFILE_TYPE}" | tr - _) + local profile + profile=$(to_upper "profile_${PROFILE_LOCATION}_${PROFILE_TYPE}" | tr - _) echo "${!profile}" } @@ -85,7 +86,8 @@ create_profile() { profile_plutil -create xml1 ;; linux) - local filename=$(profile_location) + local filename + filename=$(profile_location) profile_sudo mkdir -p "$(dirname "$filename")" echo "{}" | profile_cat "$filename" ;; @@ -117,7 +119,8 @@ export_profile() { local export="${dir}/profile.${PROFILE_LOCATION}.${PROFILE_TYPE}" case $OS in darwin | linux) - local filename=$(profile_location) + local filename + filename=$(profile_location) # Keep .plist or .json file extension cp "$filename" "${export}.${filename##*.}" ;; @@ -243,7 +246,8 @@ count_setting_segments() { # Applies $expr against the profile and updates it in-places. profile_jq() { local expr=$1 - local filename=$(profile_location) + local filename + filename=$(profile_location) # Need to use a temp file to avoid truncating the file before it has been read. jq "$expr" "$filename" | profile_cat "${filename}.tmp" profile_sudo mv "${filename}.tmp" "$filename" @@ -259,11 +263,13 @@ profile_plutil() { # Make sure all the dictionaries for the setting path exist if [[ $action =~ ^-insert|-replace|-remove$ ]]; then local setting=$2 - local count=$(count_setting_segments "$setting") + local count + count=$(count_setting_segments "$setting") if ((count > 1)); then local index for index in $(seq $((count - 1))); do - local keypath=$(echo "$setting" | cut -d . -f 1-"$index") + local keypath + keypath=$(echo "$setting" | cut -d . -f 1-"$index") # Ignore error if dictionary already exists run profile_sudo plutil -insert "$keypath" -dictionary "$(profile_location)" done @@ -285,19 +291,23 @@ profile_reg() { local action=$1 shift - local reg_key=$(profile_location) + local reg_key + reg_key=$(profile_location) if [[ $action =~ ^add|delete$ ]]; then local setting=$1 shift - local count=$(count_setting_segments "$setting") + local count + count=$(count_setting_segments "$setting") if ((count > 1)); then - local reg_subkey=$(echo "$setting" | cut -d . -f 1-"$((count - 1))") + local reg_subkey + reg_subkey=$(echo "$setting" | cut -d . -f 1-"$((count - 1))") # reg_key uses backslashes instead of dot separators reg_key="${reg_key}\\${reg_subkey//./\\}" fi - local reg_value_name=$(echo "$setting" | cut -d . -f "$count") + local reg_value_name + reg_value_name=$(echo "$setting" | cut -d . -f "$count") # reg_value_name may be empty when deleting a registry key instead of a named value if [[ -n $reg_value_name ]]; then # turn protected dots back into regular dots again diff --git a/bats/tests/helpers/utils.bash b/bats/tests/helpers/utils.bash index 7e7daf4bf97..fb9bd687d30 100644 --- a/bats/tests/helpers/utils.bash +++ b/bats/tests/helpers/utils.bash @@ -8,7 +8,8 @@ to_upper() { is_true() { # case-insensitive check; false values: '', '0', 'no', and 'false' - local value=$(to_lower "$1") + local value + value=$(to_lower "$1") if [[ $value =~ ^(0|no|false)?$ ]]; then false else @@ -176,7 +177,8 @@ update_allowed_patterns() { local enabled=$1 shift - local patterns=$(join_map ", " image_without_tag_as_json_string "$@") + local patterns + patterns=$(join_map ", " image_without_tag_as_json_string "$@") # TODO TODO TODO # Once https://github.com/rancher-sandbox/rancher-desktop/issues/4939 has been @@ -218,7 +220,8 @@ unique_filename() { capture_logs() { if capturing_logs && [ -d "$PATH_LOGS" ]; then - local logdir=$(unique_filename "${PATH_BATS_LOGS}/${RD_TEST_FILENAME}") + local logdir + logdir=$(unique_filename "${PATH_BATS_LOGS}/${RD_TEST_FILENAME}") mkdir -p "$logdir" cp -LR "$PATH_LOGS/" "$logdir" echo "${BATS_TEST_DESCRIPTION:-teardown}" >"$logdir/test_description" @@ -231,7 +234,8 @@ capture_logs() { take_screenshot() { if taking_screenshots; then if is_macos; then - local file=$(unique_filename "${PATH_BATS_LOGS}/${BATS_SUITE_TEST_NUMBER}-${BATS_TEST_DESCRIPTION}" .png) + local file + file=$(unique_filename "${PATH_BATS_LOGS}/${BATS_SUITE_TEST_NUMBER}-${BATS_TEST_DESCRIPTION}" .png) mkdir -p "$PATH_BATS_LOGS" # The terminal app must have "Screen Recording" permission; # otherwise only the desktop background is captured. diff --git a/bats/tests/helpers/vm.bash b/bats/tests/helpers/vm.bash index 4f7608ad45c..5d1bedeadf8 100644 --- a/bats/tests/helpers/vm.bash +++ b/bats/tests/helpers/vm.bash @@ -10,7 +10,8 @@ wait_for_shell() { } pkill_by_path() { - local arg=$(readlink -f "$1") + local arg + arg=$(readlink -f "$1") if [[ -n $arg ]]; then pkill -f "$arg" fi @@ -215,7 +216,8 @@ rdctl_api_settings() { } wait_for_container_engine() { - local CALLER=$(this_function) + local CALLER + CALLER=$(this_function) trace "waiting for api /settings to be callable" try --max 30 --delay 5 rdctl_api_settings diff --git a/bats/tests/k8s/helm-install-rancher.bats b/bats/tests/k8s/helm-install-rancher.bats index 500905d922a..bde8c386884 100644 --- a/bats/tests/k8s/helm-install-rancher.bats +++ b/bats/tests/k8s/helm-install-rancher.bats @@ -23,7 +23,8 @@ local_setup() { get_host() { if is_windows; then - local LB_IP=$(kubectl get svc traefik --namespace kube-system | awk 'NR==2{print $4}') + local LB_IP + LB_IP=$(kubectl get svc traefik --namespace kube-system | awk 'NR==2{print $4}') echo "$LB_IP.sslip.io" else echo "localhost" diff --git a/bats/tests/preferences/verify-paths.bats b/bats/tests/preferences/verify-paths.bats index a4c3c8416c8..e70e04a1726 100644 --- a/bats/tests/preferences/verify-paths.bats +++ b/bats/tests/preferences/verify-paths.bats @@ -2,7 +2,8 @@ load '../helpers/load' # Ensure subshells don't inherit a path that includes ~/.rd/bin -export PATH=$(echo "$PATH" | tr ':' '\n' | grep -v /.rd/bin | tr '\n' ':') +export PATH +PATH=$(echo "$PATH" | tr ':' '\n' | grep -v /.rd/bin | tr '\n' ':') local_setup() { if is_windows; then diff --git a/bats/tests/registry/creds.bats b/bats/tests/registry/creds.bats index 9a93de9eee1..9b4373d06fe 100644 --- a/bats/tests/registry/creds.bats +++ b/bats/tests/registry/creds.bats @@ -103,7 +103,8 @@ skip_for_insecure_registry() { } verify_default_credStore() { - local CREDHELPER_NAME="$(basename "$CRED_HELPER" .exe | sed s/^docker-credential-//)" + local CREDHELPER_NAME + CREDHELPER_NAME="$(basename "$CRED_HELPER" .exe | sed s/^docker-credential-//)" run jq -r .credsStore "$DOCKER_CONFIG_FILE" assert_success assert_output "$CREDHELPER_NAME" From 1a74210935ad1cd7ce90dd5abf1b1afda719ba67 Mon Sep 17 00:00:00 2001 From: Jan Dubois Date: Mon, 10 Jul 2023 23:49:54 -0700 Subject: [PATCH 4/4] Update shfmt to 3.7.0 and use ${FOO:-} for empty string defaults Changing this to ${FOO-} was done by 3.6.0, but has since been reverted. See https://github.com/mvdan/sh/issues/970 Signed-off-by: Jan Dubois --- .github/workflows/test.yaml | 2 +- bats/tests/extensions/allow-list.bats | 2 +- bats/tests/helpers/defaults.bash | 2 +- bats/tests/helpers/os.bash | 6 +++--- bats/tests/helpers/profile.bash | 2 +- bats/tests/helpers/utils.bash | 2 +- bats/tests/helpers/vm.bash | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 71478ca01ad..d0679a5240f 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -21,6 +21,6 @@ jobs: - run: npm run build - run: npm run lint:nofix - name: Install shfmt - run: go install mvdan.cc/sh/v3/cmd/shfmt@v3.6.0 + run: go install mvdan.cc/sh/v3/cmd/shfmt@v3.7.0 - run: make -C bats lint - run: npm test diff --git a/bats/tests/extensions/allow-list.bats b/bats/tests/extensions/allow-list.bats index a01a4d64f27..6017b2b1e75 100644 --- a/bats/tests/extensions/allow-list.bats +++ b/bats/tests/extensions/allow-list.bats @@ -12,7 +12,7 @@ local_setup() { } write_allow_list() { # list - local list=${1-} + local list=${1:-} local allowed=true if [ -z "$list" ]; then diff --git a/bats/tests/helpers/defaults.bash b/bats/tests/helpers/defaults.bash index 259d23de236..3f3942c3325 100644 --- a/bats/tests/helpers/defaults.bash +++ b/bats/tests/helpers/defaults.bash @@ -111,7 +111,7 @@ if using_networking_tunnel && ! is_windows; then fi ######################################################################## -if ! is_unix && [ -n "${RD_MOUNT_TYPE-}" ]; then +if ! is_unix && [ -n "${RD_MOUNT_TYPE:-}" ]; then fatal "RD_MOUNT_TYPE only works on Linux and macOS" fi diff --git a/bats/tests/helpers/os.bash b/bats/tests/helpers/os.bash index 258927a6886..8cb35e43cf4 100644 --- a/bats/tests/helpers/os.bash +++ b/bats/tests/helpers/os.bash @@ -23,7 +23,7 @@ Linux) esac is_linux() { - if [ -z "${1-}" ]; then + if [ -z "${1:-}" ]; then test "$OS" = linux else test "$OS" = linux -a "$ARCH" = "$1" @@ -31,7 +31,7 @@ is_linux() { } is_macos() { - if [ -z "${1-}" ]; then + if [ -z "${1:-}" ]; then test "$OS" = darwin else test "$OS" = darwin -a "$ARCH" = "$1" @@ -39,7 +39,7 @@ is_macos() { } is_windows() { - if [ -z "${1-}" ]; then + if [ -z "${1:-}" ]; then test "$OS" = windows else test "$OS" = windows -a "$ARCH" = "$1" diff --git a/bats/tests/helpers/profile.bash b/bats/tests/helpers/profile.bash index 8518a33653e..105f8ced203 100644 --- a/bats/tests/helpers/profile.bash +++ b/bats/tests/helpers/profile.bash @@ -343,6 +343,6 @@ ensure_profile_is_deleted() { # Only run this once per test file. It cannot be part of setup_file() because # we want to be able to call fatal() and skip the rest of the tests. -if [[ -z ${BATS_SUITE_TEST_NUMBER-} ]] && deleting_profiles; then +if [[ -z ${BATS_SUITE_TEST_NUMBER:-} ]] && deleting_profiles; then foreach_profile ensure_profile_is_deleted fi diff --git a/bats/tests/helpers/utils.bash b/bats/tests/helpers/utils.bash index fb9bd687d30..0bc17030c2f 100644 --- a/bats/tests/helpers/utils.bash +++ b/bats/tests/helpers/utils.bash @@ -203,7 +203,7 @@ EOF # will return /tmp/image.png, or /tmp/image_2.png, etc. unique_filename() { local basename=$1 - local extension=${2-} + local extension=${2:-} local index=1 local suffix="" diff --git a/bats/tests/helpers/vm.bash b/bats/tests/helpers/vm.bash index 5d1bedeadf8..a40603270a0 100644 --- a/bats/tests/helpers/vm.bash +++ b/bats/tests/helpers/vm.bash @@ -106,7 +106,7 @@ start_container_engine() { if using_ghcr_images; then registry="ghcr.io" fi - if is_true "${RD_USE_PROFILE-}"; then + if is_true "${RD_USE_PROFILE:-}"; then if is_windows; then # Translate any dots in the distro name into $RD_PROTECTED_DOT (e.g. "Ubuntu-22.04") # so that they are not treated as setting separator characters.