fix(hack): compare release-watch keys per struct, in both directions

hack/runner-release-watch.sh

@@ -15,6 +15,7 @@ CONFIG_FILE=${RUNNER_WATCH_CONFIG_FILE:-api/v1beta2/gitlab_types.go}
 MIRROR=${RUNNER_WATCH_MIRROR:-gitlabhq/gitlab-runner}
 GITLAB_API=${RUNNER_WATCH_GITLAB_API:-https://gitlab.com/api/v4/projects/gitlab-org%2Fgitlab-runner}
 HUB=${RUNNER_WATCH_HUB:-https://hub.docker.com/v2/repositories}
+SUPPRESS_FILE=${RUNNER_WATCH_SUPPRESS_FILE:-hack/runner-release-watch.suppress}
 DRY_RUN=${RUNNER_WATCH_DRY_RUN:-}
 
 # The pin is a Go constant rather than a dependency, so nothing else would
@@ -33,6 +34,29 @@ if [ ! -r "${CONFIG_FILE}" ]; then
   exit 1
 fi
 
+# Deliberate omissions are data the script consults, not prose in a Go comment:
+# one `Struct.key` per line, `#` comments and blank lines ignored. Parsed up
+# front so a typo stops the run rather than silently excluding nothing.
+suppress=""
+if [ -e "${SUPPRESS_FILE}" ]; then
+  # -f as well as -r: a directory is readable, and BSD sed exits 0 on one, so
+  # the parse below would report an exclusion file with zero exclusions.
+  if [ ! -f "${SUPPRESS_FILE}" ] || [ ! -r "${SUPPRESS_FILE}" ]; then
+    echo "${SUPPRESS_FILE} is not a readable file; refusing to report a config delta" >&2
+    exit 1
+  fi
+  raw=$(sed -e 's/#.*//' -e 's/[[:space:]]//g' "${SUPPRESS_FILE}") || {
+    echo "could not read ${SUPPRESS_FILE}; refusing to report a config delta" >&2
+    exit 1
+  }
+  suppress=$(printf '%s\n' "${raw}" | grep -E '.' || true)
+  malformed=$(printf '%s' "${suppress}" | grep -vE '^[A-Za-z0-9_]+\.[A-Za-z0-9_]+$' || true)
+  if [ -n "${malformed}" ]; then
+    printf 'malformed entries in %s, want Struct.key:\n%s\n' "${SUPPRESS_FILE}" "${malformed}" >&2
+    exit 1
+  fi
+fi
+
 # Field-wise so the result does not depend on GNU sort -V being present. 10# so
 # a zero-padded component cannot abort the arithmetic as an invalid octal.
 vgt() {
@@ -100,7 +124,8 @@ fi
 
 # Every struct reachable from a Kubernetes* root, not just the roots: the
 # operator mirrors the nested types too, so a key added to one of those still
-# needs a CRD field. Following references keeps that self-maintaining.
+# needs a CRD field. Emitted as `Struct.key`, because a bare name set counts a
+# key as exposed everywhere once it is exposed anywhere (see #63).
 KEYS_AWK='
 /^type [A-Za-z0-9_]+ struct \{$/ { t = $2; seen[t] = 1; next }
 /^\}$/ { t = ""; next }
@@ -123,7 +148,10 @@ END {
     split(refs[cur], r, " ")
     for (j in r) if (r[j] != "" && (r[j] in seen) && !(r[j] in done)) queue[++n] = r[j]
   }
-  for (x in done) { split(keys[x], kk, " "); for (j in kk) if (kk[j] != "") print kk[j] }
+  for (x in done) {
+    split(keys[x], kk, " ")
+    for (j in kk) if (kk[j] != "") print x "." kk[j]
+  }
 }'
 kube_keys() { awk "${KEYS_AWK}" "$1" | sort -u; }
 
@@ -139,6 +167,10 @@ by_hand() {
   echo "**Compare the executor config by hand before closing this.**"
 }
 
+bullets() { sed 's/^/- `/;s/$/`/' "$1"; }
+# wc pads on some platforms, and the count is interpolated into prose.
+count_lines() { wc -l <"$1" | tr -d '[:space:]'; }
+
 config_section() {
   local old="${tmp}/old.go" new="${tmp}/new.go"
   if ! fetch_upstream_config "${pinned}" >"${old}" 2>/dev/null ||
@@ -147,32 +179,74 @@ config_section() {
     by_hand
     return
   fi
+  kube_keys "${old}" >"${tmp}/old.keys"
+  kube_keys "${new}" >"${tmp}/new.keys"
+  kube_keys "${CONFIG_FILE}" >"${tmp}/ours.keys"
   # An empty extraction means the structs moved or were renamed. Saying "no keys
   # added" there would be the most misleading output this tool emits. All three
   # inputs are checked: a silent zero on any side skews the whole comparison.
   local f
-  for f in "${old}:v${pinned}" "${new}:v${latest}" "${CONFIG_FILE}:${CONFIG_FILE}"; do
-    if [ -z "$(kube_keys "${f%%:*}")" ]; then
+  for f in "old.keys:v${pinned}" "new.keys:v${latest}" "ours.keys:${CONFIG_FILE}"; do
+    if [ ! -s "${tmp}/${f%%:*}" ]; then
       echo "Extracted no toml keys from ${f##*:}; the config structs may have moved."
       by_hand
       return
     fi
   done
-  local added removed missing
-  added=$(comm -13 <(kube_keys "${old}") <(kube_keys "${new}") | sed 's/^/- `/;s/$/`/')
-  removed=$(comm -23 <(kube_keys "${old}") <(kube_keys "${new}") | sed 's/^/- `/;s/$/`/')
-  missing=$(comm -23 <(kube_keys "${new}") <(kube_keys "${CONFIG_FILE}") | sed 's/^/- `/;s/$/`/')
+  printf '%s' "${suppress}" | grep -E '.' | sort -u >"${tmp}/sup" || true
+
+  # Release delta, upstream against itself. Not filtered by the exclusion list:
+  # a key added to a subtree we skip is still a real upstream change, and it is
+  # reported once rather than on every release.
+  comm -13 "${tmp}/old.keys" "${tmp}/new.keys" >"${tmp}/added"
+  comm -23 "${tmp}/old.keys" "${tmp}/new.keys" >"${tmp}/removed"
+  # Exposure gap, both directions, so a key we carry that upstream deleted shows
+  # up too. Only these are filtered, because they recompute the whole backlog on
+  # every release and would otherwise repeat the same omissions forever.
+  comm -23 "${tmp}/new.keys" "${tmp}/ours.keys" >"${tmp}/missing.all"
+  comm -13 "${tmp}/new.keys" "${tmp}/ours.keys" >"${tmp}/stale.all"
+  sort -u "${tmp}/missing.all" "${tmp}/stale.all" >"${tmp}/gaps"
+  comm -23 "${tmp}/missing.all" "${tmp}/sup" >"${tmp}/missing"
+  comm -23 "${tmp}/stale.all" "${tmp}/sup" >"${tmp}/stale"
+  comm -12 "${tmp}/gaps" "${tmp}/sup" >"${tmp}/sup.used"
+  comm -13 "${tmp}/gaps" "${tmp}/sup" >"${tmp}/sup.unused"
 
-  printf 'Upstream executor toml keys, v%s to v%s:\n\n' "${pinned}" "${latest}"
-  if [ -n "${added}" ]; then
-    printf '### Added upstream, so likely new CRD fields\n\n%s\n\n' "${added}"
+  local excluded
+  if [ -e "${SUPPRESS_FILE}" ]; then
+    excluded=$(printf 'with %s of them excluded by `%s`' \
+      "$(count_lines "${tmp}/sup.used")" "${SUPPRESS_FILE}")
+  else
+    excluded=$(printf 'with no exclusion file at `%s`' "${SUPPRESS_FILE}")
+  fi
+  # State the comparison performed. "Nothing unexposed" is worth very little
+  # without it, and the flat-name version of this check read as a proof it was
+  # not: it reported zero while four placements were genuinely missing (#63).
+  printf 'Upstream executor toml keys, v%s to v%s. Compared as (struct, key) pairs rather than bare key names: %s upstream, %s here. ' \
+    "${pinned}" "${latest}" "$(count_lines "${tmp}/new.keys")" "$(count_lines "${tmp}/ours.keys")"
+  printf 'The exposure check runs in both directions, %s.\n\n' "${excluded}"
+
+  if [ -s "${tmp}/added" ]; then
+    printf '### Added upstream, so likely new CRD fields\n\n%s\n\n' "$(bullets "${tmp}/added")"
   else
     printf '### No keys added upstream\n\nNo new CRD fields needed for the executor config.\n\n'
   fi
-  [ -n "${removed}" ] && printf '### Removed upstream, so possibly dead here\n\n%s\n\n' "${removed}"
-  if [ -n "${missing}" ]; then
+  if [ -s "${tmp}/removed" ]; then
+    printf '### Removed upstream, so possibly dead here\n\n%s\n\n' "$(bullets "${tmp}/removed")"
+  fi
+  if [ -s "${tmp}/missing" ]; then
     printf '### In upstream v%s but not exposed by `%s`\n\n%s\n\n' \
-      "${latest}" "${CONFIG_FILE}" "${missing}"
+      "${latest}" "${CONFIG_FILE}" "$(bullets "${tmp}/missing")"
+  else
+    printf '### Every upstream key is exposed\n\nNo (struct, key) pair in v%s is missing from `%s`.\n\n' \
+      "${latest}" "${CONFIG_FILE}"
+  fi
+  if [ -s "${tmp}/stale" ]; then
+    printf '### Exposed by `%s` but gone from upstream v%s\n\n%s\n\n' \
+      "${CONFIG_FILE}" "${latest}" "$(bullets "${tmp}/stale")"
+  fi
+  if [ -s "${tmp}/sup.unused" ]; then
+    printf '### Exclusions in `%s` that matched nothing\n\n%s\n\n' \
+      "${SUPPRESS_FILE}" "$(bullets "${tmp}/sup.unused")"
   fi
 }
 

hack/runner-release-watch.suppress

@@ -0,0 +1,20 @@
+# Deliberate omissions for hack/runner-release-watch.sh: one `Struct.key` per
+# line, `#` comments and blank lines ignored. An entry matching neither side of
+# the comparison is reported as unused, so a stale line cannot hide here.
+
+# The Kubernetes autoscaler subtree is not exposed on purpose (see #58): pause
+# pods do nothing without a cluster autoscaler, `preemptive_mode` is dead config
+# at v19.2.2, and the Deployments it creates carry no owner reference.
+KubernetesConfig.autoscaler
+KubernetesAutoscalerConfig.max_pause_pods
+KubernetesAutoscalerConfig.pause_pod_image
+KubernetesAutoscalerConfig.pause_pod_image_pull_secrets
+KubernetesAutoscalerConfig.pause_pod_priority_class_name
+KubernetesAutoscalerConfig.policy
+AutoscalerPolicyConfig.idle_count
+AutoscalerPolicyConfig.idle_time
+AutoscalerPolicyConfig.periods
+AutoscalerPolicyConfig.preemptive_mode
+AutoscalerPolicyConfig.scale_factor
+AutoscalerPolicyConfig.scale_factor_limit
+AutoscalerPolicyConfig.timezone

hack/runner-release-watch_test.sh

@@ -141,6 +141,16 @@ in_section() {
   fi
 }
 
+not_in_section() {
+  local name=$1 needle=$2 file=$3
+  if awk -v h="### ${name}" '$0 ~ "^"h {f=1;next} /^### /{f=0} f' "${file}" \
+    | grep -qF -- "${needle}"; then
+    echo "  FAIL: '${needle}' under '${name}'"; fail=$((fail + 1))
+  else
+    echo "  ok: '${needle}' not under '${name}'"; pass=$((pass + 1))
+  fi
+}
+
 mk_types() {
   printf 'const DefaultRunnerImage = "gitlab/gitlab-runner:alpine-v%s"\n' "$1" > "${ROOT}/types.go"
 }
@@ -163,6 +173,25 @@ mk_config_at() {
     echo '}'
   } > "${out}"
 }
+# Same three structs, but each nested struct's key is set independently, so a
+# name can sit on one struct and not another. A flat name set cannot see that
+# gap; a (struct, key) set can.
+mk_config_pairs() {
+  local out=$1 csi=$2 ref=$3; shift 3
+  { echo 'type KubernetesConfig struct {'
+    printf '\tCSI KubernetesCSI `toml:"csi,omitempty"`\n'
+    printf '\tSvc Referenced `toml:"svc,omitempty"`\n'
+    for k in "$@"; do printf '\tField string `toml:"%s,omitempty"`\n' "${k}"; done
+    echo '}'
+    echo 'type KubernetesCSI struct {'
+    printf '\tField string `toml:"%s,omitempty"`\n' "${csi}"
+    echo '}'
+    echo 'type Referenced struct {'
+    printf '\tField string `toml:"%s,omitempty"`\n' "${ref}"
+    echo '}'
+    printf 'Set helper image flavor (alpine, ubuntu)\n'
+  } > "${out}"
+}
 
 export GH_CALLS="${ROOT}/calls.log"
 export GH_BODY_CAPTURE="${ROOT}/body.md"
@@ -176,6 +205,9 @@ export GH_CONFIG_NEW="${ROOT}/new.go"
 export HUB_FIXTURE="${ROOT}/hub.json"
 export RUNNER_WATCH_TYPES_FILE="${ROOT}/types.go"
 export RUNNER_WATCH_CONFIG_FILE="${ROOT}/ours.go"
+# Pinned at a fixture, or a run from the repo root would read the real
+# exclusion list and a run from anywhere else would not.
+export RUNNER_WATCH_SUPPRESS_FILE="${ROOT}/suppress"
 
 reset() {
   : > "${GH_CALLS}"; : > "${GH_BODY_CAPTURE}"; : > "${GH_TITLE_CAPTURE}"
@@ -195,6 +227,8 @@ reset() {
   mk_config_at "${GH_CONFIG_NEW}" nested_shared shared_key brand_new_key
   mk_config_at "${ROOT}/ours.go" nested_shared shared_key
   printf 'Set helper image flavor (alpine, ubuntu)\n' >> "${ROOT}/ours.go"
+  : > "${ROOT}/suppress"
+  export RUNNER_WATCH_SUPPRESS_FILE="${ROOT}/suppress"
   unset GH_CONFIG_FAIL HUB_FAIL GITLAB_FAIL GH_LABEL_FAIL GH_SEARCH_FAIL
   unset RUNNER_WATCH_DRY_RUN GH_TRACKED_STATE
   export PINNED_V=19.1.0
@@ -220,9 +254,9 @@ check "picks the numeric maximum" "v19.10.0" "${GH_BODY_CAPTURE}"
 absent "ignores rc tags" "19.11.0" "${GH_BODY_CAPTURE}"
 check "carries the marker" "<!-- runner-release: v19.10.0 -->" "${GH_BODY_CAPTURE}"
 check "has an added heading" "### Added upstream" "${GH_BODY_CAPTURE}"
-in_section "Added upstream" "brand_new_key" "${GH_BODY_CAPTURE}"
+in_section "Added upstream" "KubernetesConfig.brand_new_key" "${GH_BODY_CAPTURE}"
 check "has a removed heading" "### Removed upstream" "${GH_BODY_CAPTURE}"
-in_section "Removed upstream" "gone_upstream" "${GH_BODY_CAPTURE}"
+in_section "Removed upstream" "KubernetesConfig.gone_upstream" "${GH_BODY_CAPTURE}"
 absent "no false 'nothing added'" "No keys added upstream" "${GH_BODY_CAPTURE}"
 check "reports helper flavours" "ubuntu" "${GH_BODY_CAPTURE}"
 absent "flavours exclude arch tokens" '- `x86_64`' "${GH_BODY_CAPTURE}"
@@ -244,10 +278,75 @@ echo "case 3: nested Kubernetes* types are in scope"
 reset; mk_types 19.1.0
 mk_config_at "${GH_CONFIG_NEW}" nested_added shared_key
 out=$("${SCRIPT}" 2>&1)
-check "diffs a nested-type key" "nested_added" "${GH_BODY_CAPTURE}"
+check "diffs a nested-type key" "KubernetesCSI.nested_added" "${GH_BODY_CAPTURE}"
 # Referenced is reachable only through a field type, so a roots-only extraction
 # would miss it entirely.
-check "follows type references" "nested_added_via_ref" "${GH_BODY_CAPTURE}"
+check "follows type references" "Referenced.nested_added_via_ref" "${GH_BODY_CAPTURE}"
+
+echo "case 3b: a per-struct gap a flat name set cannot see"
+reset; mk_types 19.1.0
+# `shared_key` is present in both files, so a bare-name comparison is satisfied.
+# Only upstream puts it on KubernetesCSI, and only we put `other_key` there.
+mk_config_pairs "${GH_CONFIG_NEW}" shared_key ref_key shared_key
+mk_config_pairs "${ROOT}/ours.go" other_key ref_key shared_key
+out=$("${SCRIPT}" 2>&1)
+in_section "In upstream v19.10.0 but not exposed by" "KubernetesCSI.shared_key" \
+  "${GH_BODY_CAPTURE}"
+in_section "Exposed by" "KubernetesCSI.other_key" "${GH_BODY_CAPTURE}"
+# The exposed placement of the same name is reported nowhere, which is the
+# whole point: the pair is what is compared, not the name.
+absent "does not flag the exposed placement" "KubernetesConfig.shared_key" "${GH_BODY_CAPTURE}"
+check "states the comparison performed" "Compared as (struct, key) pairs" "${GH_BODY_CAPTURE}"
+
+echo "case 3c: an exclusion drops a pair from both directions"
+reset; mk_types 19.1.0
+mk_config_pairs "${GH_CONFIG_NEW}" shared_key ref_key shared_key
+mk_config_pairs "${ROOT}/ours.go" other_key ref_key shared_key
+printf '# why\nKubernetesCSI.shared_key\nKubernetesCSI.other_key\n' > "${ROOT}/suppress"
+out=$("${SCRIPT}" 2>&1)
+not_in_section "In upstream" "KubernetesCSI.shared_key" "${GH_BODY_CAPTURE}"
+check "reports full exposure instead" "Every upstream key is exposed" "${GH_BODY_CAPTURE}"
+absent "drops the stale section too" "but gone from upstream" "${GH_BODY_CAPTURE}"
+check "keeps the exclusion visible" "with 2 of them excluded by" "${GH_BODY_CAPTURE}"
+# Upstream-against-itself is not filtered: a key added to a skipped subtree is
+# still a real upstream change, and it is reported once, not every release.
+in_section "Added upstream" "KubernetesCSI.shared_key" "${GH_BODY_CAPTURE}"
+
+echo "case 3d: an exclusion that matches nothing is reported"
+reset; mk_types 19.1.0
+printf 'KubernetesCSI.no_such_key\n' > "${ROOT}/suppress"
+out=$("${SCRIPT}" 2>&1)
+check "flags the dead entry" "that matched nothing" "${GH_BODY_CAPTURE}"
+in_section "Exclusions in" "KubernetesCSI.no_such_key" "${GH_BODY_CAPTURE}"
+
+echo "case 3e: a malformed exclusion refuses before filing"
+reset; mk_types 19.1.0
+printf '# comment\nnot-a-pair\n' > "${ROOT}/suppress"
+out=$("${SCRIPT}" 2>&1); rc=$?
+rc_is "exits non-zero" "$(nonzero "${rc}")" "nonzero"
+printf '%s\n' "${out}" > "${ROOT}/o3e"
+check "names the file" "malformed entries in" "${ROOT}/o3e"
+check "quotes the entry" "not-a-pair" "${ROOT}/o3e"
+absent "does not create" "issue create" "${GH_CALLS}"
+
+echo "case 3f: no exclusion file is stated, not silently treated as empty"
+reset; mk_types 19.1.0; rm -f "${ROOT}/suppress"
+out=$("${SCRIPT}" 2>&1); rc=$?
+rc_is "exits 0" "${rc}" 0
+check "still creates an issue" "issue create" "${GH_CALLS}"
+check "says the file is absent" "no exclusion file at" "${GH_BODY_CAPTURE}"
+
+echo "case 3g: a directory as the exclusion path refuses"
+reset; mk_types 19.1.0
+# A directory is readable and BSD sed exits 0 on one, so without the -f guard
+# this reports an exclusion file that excludes nothing.
+mkdir -p "${ROOT}/suppress.d"
+export RUNNER_WATCH_SUPPRESS_FILE="${ROOT}/suppress.d"
+out=$("${SCRIPT}" 2>&1); rc=$?
+rc_is "exits non-zero" "$(nonzero "${rc}")" "nonzero"
+printf '%s\n' "${out}" > "${ROOT}/o3g"
+check "says it is not a file" "is not a readable file" "${ROOT}/o3g"
+absent "does not create" "issue create" "${GH_CALLS}"
 
 echo "case 4: label-matched issue suppresses a duplicate"
 reset; mk_types 19.1.0