codeant-ai-for-open-source[bot] commented on code in PR #41396: URL: https://github.com/apache/superset/pull/41396#discussion_r3616417619
########## helm/superset/tests/test-istio.sh: ########## @@ -0,0 +1,176 @@ +#!/usr/bin/env bash +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Renders the chart with `helm template` for several `init.istio.*` value +# combinations and asserts that the relevant manifests contain (or omit) +# the expected fields. Intended to be run from the chart directory or via +# `bash helm/superset/tests/test-istio.sh` from the repo root. +# +# Covers the fix for: +# https://github.com/apache/superset/issues/25798 +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +CHART_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" + +pass=0 +fail=0 + +assert_contains() { + local label="$1" + local needle="$2" + local haystack="$3" + if grep -qF -- "${needle}" <<<"${haystack}"; then + echo " PASS: ${label}" + pass=$((pass + 1)) + else + echo " FAIL: ${label}" + echo " expected to contain: ${needle}" + fail=$((fail + 1)) + fi +} + +assert_not_contains() { + local label="$1" + local needle="$2" + local haystack="$3" + if ! grep -qF -- "${needle}" <<<"${haystack}"; then + echo " PASS: ${label}" + pass=$((pass + 1)) + else + echo " FAIL: ${label}" + echo " expected NOT to contain: ${needle}" + fail=$((fail + 1)) + fi +} + +render() { + helm template release "${CHART_DIR}" "$@" +} + +extract_init_job() { + awk ' + /^# Source: superset\/templates\/init-job\.yaml/ { capture = 1 } + capture && /^---$/ { capture = 0 } + capture { print } + ' <<<"$1" +} + +extract_config_secret() { + awk ' + /^# Source: superset\/templates\/secret-superset-config\.yaml/ { capture = 1 } + capture && /^---$/ { capture = 0 } + capture { print } + ' <<<"$1" +} + +echo "==> defaults: no istio mitigations applied" +out_default="$(render)" +init_job_default="$(extract_init_job "${out_default}")" +config_default="$(extract_config_secret "${out_default}")" +assert_not_contains "default does not set sidecar.istio.io/inject label" \ + 'sidecar.istio.io/inject' "${init_job_default}" +assert_not_contains "default initscript does not register quitquitquit trap" \ + 'quitquitquit' "${config_default}" + +echo "==> init.istio.disableSidecarInjection=true" +out_disable="$(render --set init.istio.disableSidecarInjection=true)" +init_job_disable="$(extract_init_job "${out_disable}")" +config_disable="$(extract_config_secret "${out_disable}")" +assert_contains "init job sets sidecar.istio.io/inject: \"false\"" \ + 'sidecar.istio.io/inject: "false"' "${init_job_disable}" +assert_not_contains "disableSidecarInjection alone does not add quitquitquit trap" \ + 'quitquitquit' "${config_disable}" + +echo "==> init.istio.terminateSidecarOnExit=true" +out_terminate="$(render --set init.istio.terminateSidecarOnExit=true)" +init_job_terminate="$(extract_init_job "${out_terminate}")" +config_terminate="$(extract_config_secret "${out_terminate}")" +assert_not_contains "terminateSidecarOnExit alone does not add inject label" \ + 'sidecar.istio.io/inject' "${init_job_terminate}" +assert_contains "initscript registers EXIT trap that calls quitquitquit" \ + "trap 'rc=\$?; curl -fsS -m 5 -X POST" "${config_terminate}" +assert_contains "initscript trap targets the configured quit endpoint" \ + 'http://localhost:15020/quitquitquit' "${config_terminate}" +assert_contains "initscript trap binds to the EXIT signal" \ + "' EXIT" "${config_terminate}" + +echo "==> init.istio.quitEndpoint override" +out_endpoint="$(render --set init.istio.terminateSidecarOnExit=true \ + --set init.istio.quitEndpoint=http://127.0.0.1:15020/quitquitquit)" +config_endpoint="$(extract_config_secret "${out_endpoint}")" +assert_contains "trap honours custom quitEndpoint" \ + 'http://127.0.0.1:15020/quitquitquit' "${config_endpoint}" + +echo "==> both options combined" +out_both="$(render --set init.istio.disableSidecarInjection=true \ + --set init.istio.terminateSidecarOnExit=true)" +init_job_both="$(extract_init_job "${out_both}")" +config_both="$(extract_config_secret "${out_both}")" +assert_contains "combined: inject label present" \ + 'sidecar.istio.io/inject: "false"' "${init_job_both}" +assert_contains "combined: trap present" \ + 'quitquitquit' "${config_both}" + +echo "==> existing init.podLabels are preserved alongside istio label" +out_labels="$(render --set init.istio.disableSidecarInjection=true \ + --set init.podLabels.team=data-platform)" +init_job_labels="$(extract_init_job "${out_labels}")" +assert_contains "user-supplied podLabel still rendered" \ + 'team: data-platform' "${init_job_labels}" +assert_contains "istio inject label rendered alongside" \ + 'sidecar.istio.io/inject: "false"' "${init_job_labels}" + +echo "==> init.istio explicitly overridden to null" +out_null_istio="$(render --set init.istio=null)" +init_job_null_istio="$(extract_init_job "${out_null_istio}")" +config_null_istio="$(extract_config_secret "${out_null_istio}")" +assert_not_contains "null init.istio does not set inject label" \ + 'sidecar.istio.io/inject' "${init_job_null_istio}" +assert_not_contains "null init.istio does not register quitquitquit trap" \ + 'quitquitquit' "${config_null_istio}" + +echo "==> EXIT trap propagates the script's exit code, not the notification's" +# The trap's own curl call is best-effort (`|| true`) and must not mask a Review Comment: **Suggestion:** The comment says the trap uses `|| true`, but the rendered script now uses `|| echo ... >&2`; this contradiction is misleading for future maintainers validating exit-code behavior. Update the comment to match the actual trap behavior so the test intent stays accurate. [comment mismatch] <details> <summary><b>Severity Level:</b> Minor ๐งน</summary> ```mdx - โ ๏ธ Test documentation misleads maintainers about trap exit behavior. - โ ๏ธ Future changes risk aligning code with incorrect comment. ``` </details> <details> <summary><b>Steps of Reproduction โ </b></summary> ```mdx 1. Open the test script helm/superset/tests/test-istio.sh and navigate to the comment at lines 147โ151, specifically line 148 which states: "The trap's own curl call is best-effort (`|| true`) and must not mask a". 2. From the same file, observe the logic starting at lines 99โ108 where the script renders the chart with init.istio.terminateSidecarOnExit=true and captures the rendered secret into config_terminate using extract_config_secret. 3. Still in helm/superset/tests/test-istio.sh, inspect the code at lines 152โ158 where quit_endpoint_line and trap_line are obtained from config_terminate and then evaluated in a subshell to exercise the rendered EXIT trap behavior. 4. Render the chart manually (or run this test) and inspect the rendered init script in the secret-superset-config.yaml manifest; the EXIT trap line obtained as trap_line uses a best-effort curl followed by a logging fallback (for example, || echo ... >> stderr) rather than the literal || true mentioned in the comment at line 148, showing the comment is out of sync with the actual trap behavior and therefore misleading documentation rather than a runtime bug. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=049b8854ee9245d1802ea95969d53156&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=049b8854ee9245d1802ea95969d53156&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** helm/superset/tests/test-istio.sh **Line:** 148:148 **Comment:** *Comment Mismatch: The comment says the trap uses `|| true`, but the rendered script now uses `|| echo ... >&2`; this contradiction is misleading for future maintainers validating exit-code behavior. Update the comment to match the actual trap behavior so the test intent stays accurate. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41396&comment_hash=21aa43e202a37b523e4b6183846e78b727a7ead58ca22cd23aa90cb307182167&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41396&comment_hash=21aa43e202a37b523e4b6183846e78b727a7ead58ca22cd23aa90cb307182167&reaction=dislike'>๐</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
