Reorder config precedence: global flag beats environment variable (#1308)

* Make --client-version flag override CLIENT_VERSION env var

The pmm-framework resolved CLIENT_VERSION from the environment before the
global --client-version flag, so a run passing --client-version=3.5.0 with
CLIENT_VERSION=3.6.0 exported (e.g. by CI) still installed 3.6.0. An explicit
CLI flag must beat an ambient env var: reorder resolve_value() to check
GLOBAL_CLIENT_VERSION first for the CLIENT_VERSION key. Update the precedence
test and the README/ARCHITECTURE docs to match.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0111eoWF5ABrxakfE2AbFsEk
Signed-off-by: Claude <noreply@anthropic.com>

* Address review: trim narrating comments from precedence fix

Drop the rationale paragraph from resolve_value() (the precedence list
already states the flag wins; the why lives in the PR/commit) and the
per-assertion comments from the cli.bats precedence test, matching the
repo's minimal-comments house style and the file's comment-free tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0111eoWF5ABrxakfE2AbFsEk
Signed-off-by: Claude <noreply@anthropic.com>

---------

Signed-off-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: peterSirotnak

qa-integration/pmm_qa/pmm-framework/ARCHITECTURE.md

@@ -168,7 +168,7 @@ Four sources can supply a value. Highest wins:
 
 ```mermaid
 flowchart LR
-    A["1. environment variable<br/>SETUP_TYPE=gr ./pmm-framework ..."] --> B["2. global flag<br/>--client-version<br/>(CLIENT_VERSION only)"]
+    A["1. global flag<br/>--client-version<br/>(CLIENT_VERSION only)"] --> B["2. environment variable<br/>SETUP_TYPE=gr ./pmm-framework ..."]
     B --> C["3. spec option<br/>--database ps,SETUP_TYPE=gr"]
     C --> D["4. registered default<br/>lib/config.sh"]
 ```

qa-integration/pmm_qa/pmm-framework/README.md

@@ -112,11 +112,14 @@ and `sentinel`/`sentinels` for Valkey.
 
 Configuration precedence is:
 
-1. Environment variable
-2. Global `--client-version` (for `CLIENT_VERSION`)
+1. Global `--client-version` flag (for `CLIENT_VERSION`)
+2. Environment variable
 3. Per-database option
 4. Registered default
 
+The `--client-version` flag comes first so an explicit override beats an
+ambient `CLIENT_VERSION` in the environment (e.g. one exported by CI).
+
 Each registration in `lib/config.sh` pins the version used when a spec omits
 one with an explicit `DEFAULT_VERSION=` entry. It is not a user-settable
 option, and the order of the version list carries no meaning. Registering

qa-integration/pmm_qa/pmm-framework/lib/config.sh

@@ -205,12 +205,12 @@ database_default_value() {
 #   e.g. setup_type=$(resolve_value PS SETUP_TYPE DB_CONFIG)
 #
 # Precedence, highest first:
-#   1. an existing shell/environment variable named KEY
-#   2. the global --client-version, for KEY == CLIENT_VERSION only
+#   1. the global --client-version flag, for KEY == CLIENT_VERSION only
+#   2. an existing shell/environment variable named KEY
 #   3. the per-database option parsed from the --database spec
 #   4. the default registered above
 #
-# Step 1 mirrors the Python framework's `os.environ.get(KEY)`, so an exported
+# Step 2 mirrors the Python framework's `os.environ.get(KEY)`, so an exported
 # but *empty* variable deliberately wins and yields ''. Contrast with
 # resolved_version() in lib/runners.sh, which mirrors `os.getenv(...) or ...`
 # and therefore skips empty values -- the two rules are intentionally
@@ -222,10 +222,10 @@ database_default_value() {
 resolve_value() {
   local type=$1 key=$2 config_name=$3
   local -n config_ref=$config_name
-  if [[ -v $key ]]; then
-    printf '%s' "${!key}"
-  elif [[ $key == CLIENT_VERSION && -n ${GLOBAL_CLIENT_VERSION:-} ]]; then
+  if [[ $key == CLIENT_VERSION && -n ${GLOBAL_CLIENT_VERSION:-} ]]; then
     printf '%s' "$GLOBAL_CLIENT_VERSION"
+  elif [[ -v $key ]]; then
+    printf '%s' "${!key}"
   elif [[ -v "config_ref[$key]" ]]; then
     printf '%s' "${config_ref[$key]}"
   else

qa-integration/pmm_qa/pmm-framework/tests/cli.bats

@@ -38,17 +38,21 @@ load helpers/test_helper
   [[ $(resolved_version PS_VERSION PS "$DB_VERSION") == 8.0 ]]
 }
 
-@test "value precedence is environment then global then database then default" {
+@test "value precedence is global flag then environment then database then default" {
   parse_database_spec 'ps,CLIENT_VERSION=from-spec,QUERY_SOURCE=slowlog'
   GLOBAL_CLIENT_VERSION=from-global
   [[ $(resolve_value PS CLIENT_VERSION DB_CONFIG) == from-global ]]
   [[ $(resolve_value PS QUERY_SOURCE DB_CONFIG) == slowlog ]]
 
   CLIENT_VERSION=from-env
+  [[ $(resolve_value PS CLIENT_VERSION DB_CONFIG) == from-global ]]
+
+  GLOBAL_CLIENT_VERSION=''
   [[ $(resolve_value PS CLIENT_VERSION DB_CONFIG) == from-env ]]
   unset CLIENT_VERSION
 
-  GLOBAL_CLIENT_VERSION=''
+  [[ $(resolve_value PS CLIENT_VERSION DB_CONFIG) == from-spec ]]
+
   unset 'DB_CONFIG[QUERY_SOURCE]'
   [[ $(resolve_value PS QUERY_SOURCE DB_CONFIG) == perfschema ]]
 }