[WIP] MG-244: OTE migration for Support Log Gather#337
Open
KeenonLee wants to merge 1 commit into
Open
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduced a comprehensive OpenShift extension test runner infrastructure, including a Docker build stage for packaging test extensions, build system targets for compiling and embedding test fixtures, Go entry point for executing extension tests with filtered specifications, and extensive e2e test suites for must-gather operator functionality with fixture templates for various scenarios (secrets, persistence, proxies, timeouts). Module dependencies were updated to include testing frameworks, OpenShift test extensions, and Kubernetes/Cobra libraries. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
KeenonLee
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@KeenonLee: This pull request references MG-244 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: KeenonLee The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (12)
test/e2e/extension/testdata/eso/pushsecret-aws-secretkey.yaml (1)
11-11: RenameREFREASHINTERVALtoREFRESHINTERVALto prevent template misuse.Line 11 and Line 25 use a misspelled parameter name. It works internally now, but it increases caller error risk and hurts maintainability.
Proposed fix
- refreshInterval: "${REFREASHINTERVAL}" + refreshInterval: "${REFRESHINTERVAL}" @@ - - name: REFREASHINTERVAL + - name: REFRESHINTERVAL value: "1m"Also applies to: 25-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/eso/pushsecret-aws-secretkey.yaml` at line 11, Replace the misspelled template variable REFREASHINTERVAL with the correct REFRESHINTERVAL everywhere it's used (e.g., in pushsecret-aws-secretkey.yaml and any related templates or callers); update the YAML key values and any references or environment variable names that supply this parameter so they match the corrected identifier, and run a quick search to ensure no remaining occurrences of REFREASHINTERVAL remain.test/e2e/extension/testdata/eso/helm-vault-config.yaml (1)
1-30: Extract duplicate Helm values configuration to shared location.The
helm-vault-config.yamlfiles intest/e2e/extension/testdata/eso/andtest/e2e/extension/testdata/sscsi/are identical. Extract to a single shared configuration to prevent drift and reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/eso/helm-vault-config.yaml` around lines 1 - 30, The two identical helm-vault-config.yaml instances should be consolidated: extract the YAML block shown (global:, injector:, server: ... ui = true) into a single shared config (e.g., helm-vault-config.yaml in a common testdata/shared location) and update the copies under test/e2e/extension/testdata/eso/ and test/e2e/extension/testdata/sscsi/ to reference or include the shared file (via your test framework's include mechanism, a kustomize/helm values merge, or a lightweight symlink) so both tests use the single source of truth; ensure filenames remain helm-vault-config.yaml in references and preserve the exact keys (global, injector, server, server.standalone.config) when merging.test/e2e/extension/testdata/mustgather/reader-pod.yaml (1)
18-18: Pin the reader image to an immutable tag or digest.Using
:latestmakes test behavior non-deterministic over time and can cause flaky e2e runs.As per coding guidelines "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Proposed change
- image: registry.access.redhat.com/ubi9/ubi-minimal:latest + image: registry.access.redhat.com/ubi9/ubi-minimal:9.5 # or preferably: +# image: registry.access.redhat.com/ubi9/ubi-minimal@sha256:<digest>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/mustgather/reader-pod.yaml` at line 18, The YAML uses an unstable image tag "registry.access.redhat.com/ubi9/ubi-minimal:latest" in reader-pod.yaml which can cause flaky e2e tests; update the image field to a pinned immutable reference (either a specific released tag like "registry.access.redhat.com/ubi9/ubi-minimal:9.2-xx" or a digest form "registry.access.redhat.com/ubi9/ubi-minimal@sha256:<digest>") so that the image in the manifest is deterministic and repeatable.test/e2e/extension/testdata/eso/externalsecret-generator.yaml (1)
11-11: Typo in parameter name:REFREASHINTERVAL→REFRESHINTERVALSame spelling error as in
externalsecret-gcpsm.yaml.Suggested fix
spec: - refreshInterval: "${REFREASHINTERVAL}" + refreshInterval: "${REFRESHINTERVAL}"- name: NAME - - name: REFREASHINTERVAL + - name: REFRESHINTERVAL value: "1m"Also applies to: 23-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/eso/externalsecret-generator.yaml` at line 11, There's a typo in the environment variable name used for refreshInterval: replace all occurrences of REFREASHINTERVAL with the correct REFRESHINTERVAL (e.g., the line setting refreshInterval: "${REFREASHINTERVAL}" and the other occurrences reported at lines 23-24), and ensure the same correction is applied in the corresponding externalsecret-gcpsm.yaml usage so the template resolves the intended REFRESHINTERVAL variable.test/e2e/extension/testdata/eso/externalsecret-gcpsm.yaml (1)
11-11: Typo in parameter name:REFREASHINTERVAL→REFRESHINTERVALThe parameter name has a spelling error that affects readability and could cause confusion when using this template.
Suggested fix
spec: - refreshInterval: "${REFREASHINTERVAL}" + refreshInterval: "${REFRESHINTERVAL}"- name: NAME - - name: REFREASHINTERVAL + - name: REFRESHINTERVAL value: "1m"Also applies to: 28-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/eso/externalsecret-gcpsm.yaml` at line 11, Replace the misspelled parameter REFREASHINTERVAL with REFRESHINTERVAL wherever it's used in this YAML template (specifically in the refreshInterval fields) so the template uses the correctly spelled variable name; update all occurrences (including the other instance mentioned) to maintain consistency.test/e2e/extension/testdata/eso/pushsecret-aws.yaml (2)
4-4: Template name mismatch: createsPushSecretbut namedexternalsecret-templateConsider renaming to
pushsecret-templatefor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/eso/pushsecret-aws.yaml` at line 4, The template name in test/e2e/extension/testdata/eso/pushsecret-aws.yaml is misleading: the resource Kind is PushSecret but the metadata.name is "externalsecret-template"; update the metadata.name value to "pushsecret-template" to match the PushSecret resource and clarify intent (look for the PushSecret Kind and the metadata.name field in that file).
11-11: Typo in parameter name:REFREASHINTERVAL→REFRESHINTERVALSame spelling error as in other ESO templates.
Also applies to: 24-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/eso/pushsecret-aws.yaml` at line 11, Replace the misspelled environment variable REFREASHINTERVAL with the correct REFRESHINTERVAL in the template where refreshInterval: "${REFREASHINTERVAL}" appears; update all other ESO templates that contain the same typo (the other occurrences noted around lines 24-25) so they use "${REFRESHINTERVAL}" instead.test/e2e/extension/testdata/sscsi/75963/gcpproviderplugin.yaml (1)
59-59: Pinbusyboximage to a specific tag for reproducibilityUsing an untagged image defaults to
latest, which can cause non-reproducible test runs.Suggested fix
- name: chown-provider-mount - image: busybox + image: busybox:1.36 command:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/sscsi/75963/gcpproviderplugin.yaml` at line 59, Replace the untagged image reference "image: busybox" with a pinned tag (for example "image: busybox:1.36.1" or another explicit, tested version) in the gcpproviderplugin.yaml so test runs are reproducible; update any related manifests that reference the same "busybox" symbol to use the same pinned tag.test/e2e/extension/utils.go (2)
318-320: Typo: "deprecateded".if enableTLS { - e2e.Logf("enabling TLS traffic is deprecateded") + e2e.Logf("enabling TLS traffic is deprecated") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/utils.go` around lines 318 - 320, Fix the typo in the log message emitted when enableTLS is true: update the string passed to e2e.Logf in the conditional that checks enableTLS so it reads "enabling TLS traffic is deprecated" (or a clearer phrase like "TLS traffic is deprecated") instead of "deprecateded".
94-98: Potential duplicate role bindings.This always appends a new binding for the role even if one already exists. Consider adding the member to an existing binding with the same role instead.
if add { + // Check if binding for this role already exists + found := false + for _, b := range policy.Bindings { + if b.Role == role { + b.Members = append(b.Members, member) + found = true + break + } + } + if !found { + policy.Bindings = append(policy.Bindings, &gcpcrm.Binding{ + Role: role, + Members: []string{member}, + }) + } - policy.Bindings = append(policy.Bindings, &gcpcrm.Binding{ - Role: role, - Members: []string{member}, - }) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/utils.go` around lines 94 - 98, The code currently always appends a new gcpcrm.Binding when add is true, causing duplicate bindings for the same role; instead, in the function that manipulates policy.Bindings locate an existing binding where b.Role == role (iterate policy.Bindings), and if found add member to that binding's Members slice only if it's not already present; if no such binding exists, append a new &gcpcrm.Binding{Role: role, Members: []string{member}} as you do now. Ensure you update the binding in-place on policy.Bindings and deduplicate members when adding.test/e2e/extension/testdata/sscsi/73739/vaultproviderplugin.yaml (2)
114-115: Privileged container noted.Static analysis flags the
privileged: truesetting. This is likely intentional for the CSI provider to access host paths, but consider addingallowPrivilegeEscalation: falseandreadOnlyRootFilesystem: trueif the provider supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/sscsi/73739/vaultproviderplugin.yaml` around lines 114 - 115, The securityContext currently sets privileged: true which is flagged by static analysis; update the container's securityContext (the block containing privileged: true) to also set allowPrivilegeEscalation: false and readOnlyRootFilesystem: true where supported by the CSI provider, and validate the provider's startup/host-access requirements so these restrictions don't break functionality.
110-111: Emptytolerationsfield.The
tolerations:key is defined but has no values. Either add the required tolerations or remove the empty key entirely.serviceAccountName: vault-csi-provider - tolerations: containers:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/sscsi/73739/vaultproviderplugin.yaml` around lines 110 - 111, Remove the empty tolerations key or populate it with the required toleration entries: locate the bare "tolerations:" key in vaultproviderplugin.yaml (near the "containers:" block) and either delete that line if no tolerations are needed or replace it with a proper list of toleration objects (e.g., - key:, operator:, effect:, etc.) so the manifest is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/extension/main.go`:
- Around line 28-30: Move the framework finalization into a Cobra
PersistentPreRun and integrate the stdlib flags into the root command: call
util.InitStandardFlags() as before, then add flag.CommandLine to the root Cobra
flags using rootCmd.PersistentFlags().AddGoFlagSet(flag.CommandLine) so the k8s
framework flags get parsed by Cobra, and relocate the
framework.AfterReadingAllFlags(&framework.TestContext) call into
rootCmd.PersistentPreRun (or append it to any existing PersistentPreRun chain)
so AfterReadingAllFlags runs after Cobra has parsed CLI overrides (instead of
calling it before root.Execute()).
In `@go.mod`:
- Line 27: The go.mod has a version mismatch: the require line specifies
k8s.io/kubernetes v1.34.1 while the replace maps it to
github.com/openshift/kubernetes v1.30.1-0.20251017123720-96593f323733; update
the replace entry to use a compatible v1.34.1 OpenShift fork (or remove the
replace) so the required module and the replacement are aligned with the rest of
the k8s.io modules (ensure the replace target version matches v1.34.1 or drop
the replace and vendor a compatible module), adjusting the replace statement
referencing github.com/openshift/kubernetes accordingly.
- Line 7: go.mod pins github.com/onsi/ginkgo/v2 at v2.25.1 but a replace
directive targets an OpenShift fork at v2.6.1-0... which may be incompatible;
verify intent and compatibility by confirming the replace is deliberate,
auditing all uses of Ginkgo v2 APIs (Describe, Context, BeforeEach, BeforeAll,
It, By, Ordered, etc.) across test/e2e and test/ directories for APIs or
behaviors introduced after v2.6.1, and either update the replace to a
fork/version that provides the same or newer public API as v2.25.1 or change the
pinned module version to match the replaced fork; if the replace must remain,
add a short compatibility note in go.mod or repository docs explaining why the
older fork is acceptable and list any tested Ginkgo features to avoid surprises.
In `@test/e2e/extension/mustgather.go`:
- Around line 93-96: The test logs raw proxy environment variables (httpProxy,
httpsProxy, noProxy) using e2e.Logf which may contain userinfo credentials;
create a small sanitizer and use it before logging: add a helper like
redactProxyURL(proxy string) string that parses the proxy URL (net/url.Parse),
strips or masks URL.User (e.g., remove userinfo or replace with "REDACTED"), and
returns the safe string; for noProxy, split on commas and apply the same
sanitizer to any entries that look like URLs or contain '@'; replace the direct
uses of httpProxy, httpsProxy, and noProxy in the e2e.Logf calls with the
sanitized outputs so no raw credentials are written to test artifacts.
- Around line 81-82: The current gate requires both proxies but later
unconditionally compares httpProxy which breaks HTTPS-only clusters; update the
initial check to skip only when neither proxy is present (use OR: check if
output contains "httpsProxy" || "httpProxy"), and modify the later assertions
(around the blocks referenced at lines 98-103 and 118-120) to only compare
httpProxy-related env vars when "httpProxy" is present in output (similarly
handle httpsProxy checks independently) so each proxy is validated only if
configured.
In `@test/e2e/extension/supportloggather_utils.go`:
- Line 35: The Local value is declared as a compile-time constant false so
skipIfNotLocal(Local) is always optimized away; change the declaration of Local
from a const to a package-level variable (e.g., var Local = false) so its value
is evaluated at runtime (and can be overridden for local runs), leaving
skipIfNotLocal(Local) to behave correctly; update any related tests or build
scripts that expect a constant accordingly.
- Around line 377-385: The check using strings.Contains(output, "exists")
wrongly matches "not exists"; update the probe logic in the block that builds
cmd and reads output (the oc.AsAdmin().Run(...).Output() call using
readerPodName and namespace) to detect existence precisely—e.g. trim whitespace
and compare equality (strings.TrimSpace(output) == "exists") or test for the
negative case first (strings.Contains(output, "not exists") -> failure) so that
only an exact "exists" result is treated as success; apply the same change to
the similar block around lines 552-558.
- Around line 66-78: The current checks ignore the error returned by
oc.AsAdmin().WithoutNamespace().Run("get") and rely on stdout containing
"NotFound", which misclassifies API/auth failures as missing catalogs; update
the logic in the block that queries DefaultCatalogSourceNamespace for
AutoReleaseCatalogSourceName/QECatalogSourceName to inspect the returned error
from Output() (the err value) first: if err != nil then if err.Error() indicates
a not-found condition (e.g., contains "NotFound" or "not found") treat the
catalog as missing and continue to the next candidate, otherwise treat it as a
real failure (log/return/fail the test) so you don't pick the wrong catalog;
apply this change for both the AutoRelease and QE queries that set
catalogSourceName/catalogSourceNamespace and ensure non-NotFound errors are
propagated instead of ignored.
In `@test/e2e/extension/testdata/eso/externalsecret-gcpsm-version.yaml`:
- Line 11: Replace the misspelled template parameter REFREASHINTERVAL with
REFRESHINTERVAL across all test fixtures (e.g., in externalsecret-*.yaml and
pushsecret-*.yaml templates) so placeholders and any test wiring use the correct
name; search for occurrences of "REFREASHINTERVAL" in the testdata and update
them to "REFRESHINTERVAL", and also update any tests or helper code that set or
reference that env/template variable to use REFRESHINTERVAL to keep names
consistent.
In `@test/e2e/extension/testdata/fixtures.go`:
- Around line 91-100: ListFixtures currently filters only names starting with
"testdata/" but AssetNames yields entries like "mustgather/mustgather.yaml", so
the loop returns empty; update ListFixtures to not require the "testdata/"
prefix — iterate all names returned by AssetNames(), append
strings.TrimPrefix(name, "testdata/") (so names without that prefix are
unchanged), then sort and return; keep the function name ListFixtures and call
to AssetNames unchanged.
In `@test/e2e/extension/testdata/sscsi/73739/spc_vault_pod.yaml`:
- Around line 5-23: The pod spec for the fixture (container name "busybox" and
volume "secrets-store-inline" using CSI driver "secrets-store.csi.k8s.io") is
missing hardened security contexts; add a pod-level securityContext and a
container.securityContext for "busybox" that enforces runAsNonRoot: true and a
non-zero runAsUser (e.g. 1000), sets allowPrivilegeEscalation: false, drops all
capabilities, sets readOnlyRootFilesystem: true, and applies a seccomp/profile
of "RuntimeDefault" (or appropriate restricted profile) so the pod passes
restricted/hardened admission policies. Ensure these fields are added alongside
the existing spec/container definitions (pod-level securityContext for fsGroup
or supplementalGroups as needed and container-level securityContext for the
busybox container).
In `@test/e2e/extension/utils.go`:
- Around line 117-129: The outer loop over bindings can overwrite bindingIndex
when the same role appears multiple times; after you find the matching role and
search its Members for the member, stop iterating further to avoid picking a
later duplicate role. Modify the loop that iterates over bindings (the code
using bindings[bIdx], role, bindingIndex, and memberIndex) to break out of the
outer loop once you set bindingIndex and (after the inner member search) set
memberIndex — e.g. perform the inner member search, then break the outer loop
(or use a labeled break/return) so later bindings with the same role cannot
overwrite the indices.
---
Nitpick comments:
In `@test/e2e/extension/testdata/eso/externalsecret-gcpsm.yaml`:
- Line 11: Replace the misspelled parameter REFREASHINTERVAL with
REFRESHINTERVAL wherever it's used in this YAML template (specifically in the
refreshInterval fields) so the template uses the correctly spelled variable
name; update all occurrences (including the other instance mentioned) to
maintain consistency.
In `@test/e2e/extension/testdata/eso/externalsecret-generator.yaml`:
- Line 11: There's a typo in the environment variable name used for
refreshInterval: replace all occurrences of REFREASHINTERVAL with the correct
REFRESHINTERVAL (e.g., the line setting refreshInterval: "${REFREASHINTERVAL}"
and the other occurrences reported at lines 23-24), and ensure the same
correction is applied in the corresponding externalsecret-gcpsm.yaml usage so
the template resolves the intended REFRESHINTERVAL variable.
In `@test/e2e/extension/testdata/eso/helm-vault-config.yaml`:
- Around line 1-30: The two identical helm-vault-config.yaml instances should be
consolidated: extract the YAML block shown (global:, injector:, server: ... ui =
true) into a single shared config (e.g., helm-vault-config.yaml in a common
testdata/shared location) and update the copies under
test/e2e/extension/testdata/eso/ and test/e2e/extension/testdata/sscsi/ to
reference or include the shared file (via your test framework's include
mechanism, a kustomize/helm values merge, or a lightweight symlink) so both
tests use the single source of truth; ensure filenames remain
helm-vault-config.yaml in references and preserve the exact keys (global,
injector, server, server.standalone.config) when merging.
In `@test/e2e/extension/testdata/eso/pushsecret-aws-secretkey.yaml`:
- Line 11: Replace the misspelled template variable REFREASHINTERVAL with the
correct REFRESHINTERVAL everywhere it's used (e.g., in
pushsecret-aws-secretkey.yaml and any related templates or callers); update the
YAML key values and any references or environment variable names that supply
this parameter so they match the corrected identifier, and run a quick search to
ensure no remaining occurrences of REFREASHINTERVAL remain.
In `@test/e2e/extension/testdata/eso/pushsecret-aws.yaml`:
- Line 4: The template name in
test/e2e/extension/testdata/eso/pushsecret-aws.yaml is misleading: the resource
Kind is PushSecret but the metadata.name is "externalsecret-template"; update
the metadata.name value to "pushsecret-template" to match the PushSecret
resource and clarify intent (look for the PushSecret Kind and the metadata.name
field in that file).
- Line 11: Replace the misspelled environment variable REFREASHINTERVAL with the
correct REFRESHINTERVAL in the template where refreshInterval:
"${REFREASHINTERVAL}" appears; update all other ESO templates that contain the
same typo (the other occurrences noted around lines 24-25) so they use
"${REFRESHINTERVAL}" instead.
In `@test/e2e/extension/testdata/mustgather/reader-pod.yaml`:
- Line 18: The YAML uses an unstable image tag
"registry.access.redhat.com/ubi9/ubi-minimal:latest" in reader-pod.yaml which
can cause flaky e2e tests; update the image field to a pinned immutable
reference (either a specific released tag like
"registry.access.redhat.com/ubi9/ubi-minimal:9.2-xx" or a digest form
"registry.access.redhat.com/ubi9/ubi-minimal@sha256:<digest>") so that the image
in the manifest is deterministic and repeatable.
In `@test/e2e/extension/testdata/sscsi/73739/vaultproviderplugin.yaml`:
- Around line 114-115: The securityContext currently sets privileged: true which
is flagged by static analysis; update the container's securityContext (the block
containing privileged: true) to also set allowPrivilegeEscalation: false and
readOnlyRootFilesystem: true where supported by the CSI provider, and validate
the provider's startup/host-access requirements so these restrictions don't
break functionality.
- Around line 110-111: Remove the empty tolerations key or populate it with the
required toleration entries: locate the bare "tolerations:" key in
vaultproviderplugin.yaml (near the "containers:" block) and either delete that
line if no tolerations are needed or replace it with a proper list of toleration
objects (e.g., - key:, operator:, effect:, etc.) so the manifest is valid.
In `@test/e2e/extension/testdata/sscsi/75963/gcpproviderplugin.yaml`:
- Line 59: Replace the untagged image reference "image: busybox" with a pinned
tag (for example "image: busybox:1.36.1" or another explicit, tested version) in
the gcpproviderplugin.yaml so test runs are reproducible; update any related
manifests that reference the same "busybox" symbol to use the same pinned tag.
In `@test/e2e/extension/utils.go`:
- Around line 318-320: Fix the typo in the log message emitted when enableTLS is
true: update the string passed to e2e.Logf in the conditional that checks
enableTLS so it reads "enabling TLS traffic is deprecated" (or a clearer phrase
like "TLS traffic is deprecated") instead of "deprecateded".
- Around line 94-98: The code currently always appends a new gcpcrm.Binding when
add is true, causing duplicate bindings for the same role; instead, in the
function that manipulates policy.Bindings locate an existing binding where
b.Role == role (iterate policy.Bindings), and if found add member to that
binding's Members slice only if it's not already present; if no such binding
exists, append a new &gcpcrm.Binding{Role: role, Members: []string{member}} as
you do now. Ensure you update the binding in-place on policy.Bindings and
deduplicate members when adding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3218da22-7cc3-4d35-86b6-b7ebc2b951dd
⛔ Files ignored due to path filters (241)
go.sumis excluded by!**/*.sumvendor/cel.dev/expr/.bazelversionis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/.gitattributesis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/CODE_OF_CONDUCT.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/GOVERNANCE.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/MAINTAINERS.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/MODULE.bazelis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/README.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/WORKSPACEis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/WORKSPACE.bzlmodis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/checked.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cel.dev/expr/cloudbuild.yamlis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/eval.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cel.dev/expr/explain.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cel.dev/expr/regen_go_proto.shis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/regen_go_proto_canonical_protos.shis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/syntax.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cel.dev/expr/value.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/CHANGES.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/README.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/auth.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/compute.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/detect.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/doc.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/filetypes.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/aws_provider.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/executable_provider.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/externalaccount.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/file_provider.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/info.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/programmatic_provider.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/url_provider.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/x509_provider.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccountuser/externalaccountuser.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/gdch/gdch.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/impersonate/idtoken.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/impersonate/impersonate.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/internal/stsexchange/sts_exchange.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/credentials/selfsignedjwt.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/grpctransport/dial_socketopt.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/grpctransport/directpath.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/grpctransport/grpctransport.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/grpctransport/pool.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/httptransport/httptransport.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/httptransport/transport.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/compute/compute.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/compute/manufacturer.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/compute/manufacturer_linux.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/compute/manufacturer_windows.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/credsfile/credsfile.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/credsfile/filetype.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/credsfile/parse.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/jwt/jwt.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/transport/cba.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/transport/cert/default_cert.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/transport/cert/enterprise_cert.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/transport/cert/secureconnect_cert.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/transport/cert/workload_cert.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/transport/s2a.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/internal/transport/transport.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/oauth2adapt/CHANGES.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/oauth2adapt/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/oauth2adapt/oauth2adapt.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/auth/threelegged.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/CHANGES.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/README.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/log.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/metadata.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/retry.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/retry_linux.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_linux.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_windows.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/iam/CHANGES.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/iam/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/iam/README.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/iam/apiv1/iampb/iam_policy.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/iam/apiv1/iampb/options.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/iam/apiv1/iampb/policy.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/iam/apiv1/iampb/resource_policy_member.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/iam/iam.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/internal/.repo-metadata-full.jsonis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/internal/README.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/internal/annotate.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/internal/gen_info.shis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/internal/optional/optional.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/internal/retry.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/internal/trace/trace.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/internal/version/update_version.shis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/alert_policy_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/auxiliary.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/auxiliary_go123.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/doc.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/gapic_metadata.jsonis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/group_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/helpers.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/metric_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/alert.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/alert_service.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/common.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/dropped_labels.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/group.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/group_service.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/metric.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/metric_service.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/mutation_record.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/notification.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/notification_service.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/query_service.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/service.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/service_service.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/snooze.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/snooze_service.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/span_context.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/uptime.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/uptime_service.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/notification_channel_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/query_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/service_monitoring_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/snooze_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/uptime_check_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/apiv3/v2/version.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/monitoring/internal/version.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/CHANGES.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/README.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/TESTING.mdis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/acl.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/bucket.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/copy.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/doc.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/dynamic_delay.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/emulator_test.shis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/experimental/experimental.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/grpc_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/grpc_dp.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/grpc_metrics.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/grpc_reader.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/grpc_reader_multi_range.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/grpc_writer.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/hmac.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/http_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/iam.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/apiv2/auxiliary.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/apiv2/auxiliary_go123.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/apiv2/doc.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/apiv2/gapic_metadata.jsonis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/apiv2/helpers.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/apiv2/storage_client.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/apiv2/storagepb/storage.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/apiv2/version.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/experimental.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/internal/version.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/invoke.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/notifications.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/option.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/post_policy_v4.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/reader.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/storage.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/storage.replayis excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/trace.gois excluded by!vendor/**,!**/vendor/**vendor/cloud.google.com/go/storage/writer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/core.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/defaultlog.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/defaultlog_syslog.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/defaultlog_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/progress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/request.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/response.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-pipeline-go/pipeline/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/LICENSE.txtis excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/NOTICE.txtis excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/profiles/latest/containerregistry/mgmt/containerregistry/models.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/LICENSE.txtis excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_identifier.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/resource_type.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_bearer_token.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_register_rp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_trace_namespace.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/ci.ymlis excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/cloud.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/core.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/etag.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/exported.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/pipeline.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/request.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/log/log.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/async/async.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/body/body.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/op/op.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/poller.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/constants.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/shared.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pipeline.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_api_version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_bearer_token.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_body_download.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_header.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_trace.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_include_response.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_key_credential.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_logging.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_request_id.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_retry.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (59)
Dockerfile.openshiftMakefilebindata.mkcmd/extension/main.gogo.modtest/e2e/extension/mustgather.gotest/e2e/extension/oap_suite.gotest/e2e/extension/olm_utils.gotest/e2e/extension/supportloggather_utils.gotest/e2e/extension/testdata/OWNERStest/e2e/extension/testdata/bindata.gotest/e2e/extension/testdata/eso/clustergenerator-password.yamltest/e2e/extension/testdata/eso/externalsecret-awsps.yamltest/e2e/extension/testdata/eso/externalsecret-awssm.yamltest/e2e/extension/testdata/eso/externalsecret-gcpsm-version.yamltest/e2e/extension/testdata/eso/externalsecret-gcpsm.yamltest/e2e/extension/testdata/eso/externalsecret-generator.yamltest/e2e/extension/testdata/eso/externalsecret-vault.yamltest/e2e/extension/testdata/eso/generator-password.yamltest/e2e/extension/testdata/eso/helm-vault-config.yamltest/e2e/extension/testdata/eso/namespace.yamltest/e2e/extension/testdata/eso/operandConfig.yamltest/e2e/extension/testdata/eso/operatorgroup.yamltest/e2e/extension/testdata/eso/pushsecret-aws-secretkey.yamltest/e2e/extension/testdata/eso/pushsecret-aws.yamltest/e2e/extension/testdata/eso/secretstore-awssm.yamltest/e2e/extension/testdata/eso/secretstore-gcpsm.yamltest/e2e/extension/testdata/eso/secretstore-vault.yamltest/e2e/extension/testdata/eso/subscription.yamltest/e2e/extension/testdata/exec-helm-helper.yamltest/e2e/extension/testdata/fixtures.gotest/e2e/extension/testdata/konflux-fbc.yamltest/e2e/extension/testdata/mustgather/mustgather-audit.yamltest/e2e/extension/testdata/mustgather/mustgather-noupload.yamltest/e2e/extension/testdata/mustgather/mustgather-proxy.yamltest/e2e/extension/testdata/mustgather/mustgather-pvc.yamltest/e2e/extension/testdata/mustgather/mustgather-retain.yamltest/e2e/extension/testdata/mustgather/mustgather-timeout.yamltest/e2e/extension/testdata/mustgather/mustgather-upload.yamltest/e2e/extension/testdata/mustgather/mustgather.yamltest/e2e/extension/testdata/mustgather/namespace.yamltest/e2e/extension/testdata/mustgather/operatorgroup.yamltest/e2e/extension/testdata/mustgather/pvc.yamltest/e2e/extension/testdata/mustgather/reader-pod.yamltest/e2e/extension/testdata/mustgather/subscription.yamltest/e2e/extension/testdata/operatorgroup.yamltest/e2e/extension/testdata/sscsi/73739/spc_vault_pod.yamltest/e2e/extension/testdata/sscsi/73739/vault_v1_secretproviderclass.yamltest/e2e/extension/testdata/sscsi/73739/vaultproviderplugin.yamltest/e2e/extension/testdata/sscsi/75963/gcpproviderplugin.yamltest/e2e/extension/testdata/sscsi/75963/invalid_sc_pod.yamltest/e2e/extension/testdata/sscsi/75963/sc_pod.yamltest/e2e/extension/testdata/sscsi/75970/sc_pod.yamltest/e2e/extension/testdata/sscsi/drivers.yamltest/e2e/extension/testdata/sscsi/helm-vault-config.yamltest/e2e/extension/testdata/sscsi/operatorgroup.yamltest/e2e/extension/testdata/sscsi/subscription.yamltest/e2e/extension/testdata/subscription.yamltest/e2e/extension/utils.go
Comment on lines
+28
to
+30
| // Initialize test framework flags (required for kubeconfig, provider, etc.) | ||
| util.InitStandardFlags() | ||
| framework.AfterReadingAllFlags(&framework.TestContext) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "main.go" -path "*/extension/*" -type fRepository: openshift/must-gather-operator
Length of output: 97
🏁 Script executed:
cat -n cmd/extension/main.goRepository: openshift/must-gather-operator
Length of output: 5809
🏁 Script executed:
rg "AfterReadingAllFlags" -B 5 -A 5Repository: openshift/must-gather-operator
Length of output: 1898
🏁 Script executed:
rg "root\.Execute\(\)" cmd/extension/main.go -B 10 -A 5Repository: openshift/must-gather-operator
Length of output: 315
🏁 Script executed:
rg "InitStandardFlags" -B 3 -A 10Repository: openshift/must-gather-operator
Length of output: 3462
🏁 Script executed:
rg "DefaultExtensionCommands" -B 2 -A 15Repository: openshift/must-gather-operator
Length of output: 2090
🏁 Script executed:
find . -path "*/test/e2e/extension/*.go" -type f | head -5Repository: openshift/must-gather-operator
Length of output: 264
🏁 Script executed:
cat -n test/e2e/extension/mustgather.go | head -50Repository: openshift/must-gather-operator
Length of output: 2071
🏁 Script executed:
rg "flag.CommandLine.Parse" cmd/extension/main.goRepository: openshift/must-gather-operator
Length of output: 56
🏁 Script executed:
cat vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/run/suite.go | head -100Repository: openshift/must-gather-operator
Length of output: 184
🏁 Script executed:
rg "ParseFlags\|flag\.Parse" -A 3 -B 3Repository: openshift/must-gather-operator
Length of output: 56
🏁 Script executed:
rg "flag\.CommandLine\.Parse|flag\.Parse" -B 5 -A 5Repository: openshift/must-gather-operator
Length of output: 11072
🏁 Script executed:
find vendor/github.com/openshift-eng/openshift-tests-extension -name "*.go" -type f | head -20Repository: openshift/must-gather-operator
Length of output: 1817
🏁 Script executed:
ls -la vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/Repository: openshift/must-gather-operator
Length of output: 528
🏁 Script executed:
cat vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.go | head -80Repository: openshift/must-gather-operator
Length of output: 2576
🏁 Script executed:
rg "PersistentPreRun\|PersistentPreRunE" vendor/github.com/openshift-eng/openshift-tests-extension -B 2 -A 10 | head -60Repository: openshift/must-gather-operator
Length of output: 56
🏁 Script executed:
python3 << 'EOF'
# Check if Cobra integrates with flag.CommandLine
# and how the flags flow between k8s framework and Cobra
import subprocess
result = subprocess.run(
['rg', 'flag.CommandLine', 'vendor/github.com/openshift-eng/openshift-tests-extension', '-B 2', '-A 5'],
capture_output=True,
text=True
)
print("Cobra extension flag.CommandLine usage:")
print(result.stdout[:2000] if result.stdout else "No results")
EOFRepository: openshift/must-gather-operator
Length of output: 124
🏁 Script executed:
cat vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/component.goRepository: openshift/must-gather-operator
Length of output: 539
🏁 Script executed:
rg "BindFlags|RegisterFlagCompletionFunc" vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags -B 2 -A 5 | head -80Repository: openshift/must-gather-operator
Length of output: 4912
🏁 Script executed:
cat vendor/github.com/spf13/cobra/command.go | grep -A 30 "func (c \*Command) Execute()"Repository: openshift/must-gather-operator
Length of output: 860
🏁 Script executed:
rg "flag\.CommandLine\.Parse|flag\.Parse\(\)" -B 5 -A 5 cmd/ vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/Repository: openshift/must-gather-operator
Length of output: 56
🏁 Script executed:
cat vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.go | grep -A 40 "cmd.Flags().AddFlagSet\|cmd.PersistentFlags\|opts.componentFlags.BindFlags"Repository: openshift/must-gather-operator
Length of output: 1008
🏁 Script executed:
python3 << 'EOF'
# Search for where component and other flags are bound to Cobra commands
import subprocess
result = subprocess.run(
['rg', 'BindFlags', 'vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/', '-B 3', '-A 3'],
capture_output=True,
text=True
)
print(result.stdout[:3000] if result.stdout else "No results")
EOFRepository: openshift/must-gather-operator
Length of output: 84
🏁 Script executed:
cat vendor/github.com/spf13/cobra/command.go | grep -A 100 "func (c \*Command) persistentPreRunE"Repository: openshift/must-gather-operator
Length of output: 56
🏁 Script executed:
cat vendor/k8s.io/kubernetes/test/e2e/framework/test_context.go | grep -B 5 -A 20 "func AfterReadingAllFlags"Repository: openshift/must-gather-operator
Length of output: 991
🏁 Script executed:
python3 << 'EOF'
# Trace the flag parsing flow
# Check if Cobra calls flag.CommandLine.Parse
import subprocess
# Check what happens when Cobra parses
result = subprocess.run(
['rg', 'flag\.CommandLine\.Parse|CommandLine\.Parse', 'vendor/github.com/spf13/cobra/', '-B 3', '-A 3'],
capture_output=True,
text=True
)
print("Cobra+flag.CommandLine integration:")
print(result.stdout[:1500] if result.stdout else "No integration found")
print("\n---\n")
# Check k8s test_context for what AfterReadingAllFlags does
result2 = subprocess.run(
['rg', 'TestContext.*kubeconfig|kubeconfig.*TestContext', '-B 2', '-A 2'],
capture_output=True,
text=True
)
print("TestContext kubeconfig:")
print(result2.stdout[:1500] if result2.stdout else "No results")
EOFRepository: openshift/must-gather-operator
Length of output: 171
🏁 Script executed:
rg "flag\.Parse\(\)|CommandLine\.Parse" cmd/ -B 5 -A 5Repository: openshift/must-gather-operator
Length of output: 56
🏁 Script executed:
cat vendor/k8s.io/kubernetes/test/e2e/framework/test_context.go | sed -n '1,100p'Repository: openshift/must-gather-operator
Length of output: 3291
🏁 Script executed:
python3 << 'EOF'
# Let's trace the actual flag parsing behavior
# Check if there's any global flag parsing that happens in the extension framework
import subprocess
# Look for where flags are actually parsed in the extension ecosystem
result = subprocess.run(
['rg', 'flag\.Parse|ParseFlags', '-t', 'go', '-B 5', '-A 5'],
capture_output=True,
text=True,
timeout=10
)
lines = result.stdout.split('\n')
# Filter to only relevant results
relevant = []
for i, line in enumerate(lines):
if any(x in line for x in ['cmd/extension', 'openshift-tests-extension', 'test context', 'kubeconfig']):
relevant.append(line)
elif 'flag.Parse' in line and i < 100: # early in codebase
start = max(0, i-5)
end = min(len(lines), i+6)
relevant.extend(lines[start:end])
print("Flag parsing in relevant code:")
print('\n'.join(relevant[:2000]))
EOFRepository: openshift/must-gather-operator
Length of output: 105
🏁 Script executed:
# Check the k8s TestContext struct to see if it reads flags directly
rg "type TestContextType struct" -A 30 vendor/k8s.io/kubernetes/test/e2e/framework/Repository: openshift/must-gather-operator
Length of output: 3109
🏁 Script executed:
# Check if flag.CommandLine is merged with Cobra's pflag anywhere
rg "AddGoFlagSet|CommandLine\.AddGoFlagSet" -B 3 -A 3Repository: openshift/must-gather-operator
Length of output: 2245
🏁 Script executed:
# Check if Cobra root command sets up pflag to include flag.CommandLine
cat vendor/github.com/spf13/cobra/command.go | grep -A 20 "func (c \*Command) execute("Repository: openshift/must-gather-operator
Length of output: 660
🏁 Script executed:
# Final check: look for any global initialization that might parse flags
rg "init\(\)" cmd/extension/ -B 2 -A 10Repository: openshift/must-gather-operator
Length of output: 56
🏁 Script executed:
# Check if flag.CommandLine is ever parsed in this flow
rg "flag\.CommandLine\.Parse|pflag\.CommandLine\.Parse\(\)" -B 5 -A 5 cmd/extension/Repository: openshift/must-gather-operator
Length of output: 56
🏁 Script executed:
# Check what RegisterCommonFlags and RegisterClusterFlags do
rg "func RegisterCommonFlags|func RegisterClusterFlags" -A 15 vendor/k8s.io/kubernetes/test/e2e/framework/ | head -80Repository: openshift/must-gather-operator
Length of output: 6847
🏁 Script executed:
# Verify: are the k8s framework flags ever actually parsed?
python3 << 'EOF'
import subprocess
import re
# Search for any invocation of flag.Parse or pflag.Parse in the extension code
result = subprocess.run(
['rg', 'flag\.Parse\(\)|pflag\.Parse\(\)', 'cmd/', '--type', 'go'],
capture_output=True,
text=True,
timeout=5
)
if result.returncode == 0:
print("Flag parsing in cmd/extension:")
print(result.stdout if result.stdout else "None found")
else:
print("No flag parsing found in cmd/extension/")
# Also check if Cobra integrates the k8s flags
result2 = subprocess.run(
['rg', 'AddGoFlagSet|CommandLine', 'cmd/extension/', '--type', 'go'],
capture_output=True,
text=True,
timeout=5
)
print("\nCobra+flag integration in extension code:")
print(result2.stdout if result2.returncode == 0 and result2.stdout else "None found")
EOFRepository: openshift/must-gather-operator
Length of output: 167
🏁 Script executed:
# Critical check: does Cobra's Execute() somehow parse flag.CommandLine?
cat vendor/github.com/spf13/cobra/command.go | grep -B 5 -A 30 "func (c \*Command) ParseFlags"Repository: openshift/must-gather-operator
Length of output: 1003
🏁 Script executed:
# Check if the extension framework integrates flag.CommandLine with Cobra
rg "pflag.CommandLine|flag.CommandLine" vendor/github.com/openshift-eng/openshift-tests-extension/ -B 3 -A 3Repository: openshift/must-gather-operator
Length of output: 56
🏁 Script executed:
# Final verification: does the root command have any PreRun or PersistentPreRun hook?
cat -n cmd/extension/main.go | sed -n '90,105p'Repository: openshift/must-gather-operator
Length of output: 427
🏁 Script executed:
# Let me verify the exact issue: are k8s framework flags integrated with Cobra's pflag?
python3 << 'EOF'
import subprocess
# Check if there's any AddGoFlagSet integration
result = subprocess.run(
['rg', 'AddGoFlagSet.*CommandLine|CommandLine.*AddGoFlagSet', '-B 5', '-A 5'],
capture_output=True,
text=True,
timeout=5
)
print("AddGoFlagSet integration anywhere:")
lines = result.stdout.split('\n')
for i, line in enumerate(lines):
if 'cmd/extension' in line or 'main.go' in line or 'root.AddCommand' in line:
start = max(0, i-3)
end = min(len(lines), i+5)
print('\n'.join(lines[start:end]))
break
if not result.stdout:
print("No AddGoFlagSet integration found")
# Also check: are the k8s flags ever actually parsed?
print("\n\nChecking for flag.Parse() or pflag.Parse() in extension code:")
result2 = subprocess.run(
['rg', 'flag\.Parse\(\)|pflag\.Parse\(\)', 'cmd/extension', '--type', 'go'],
capture_output=True,
text=True,
timeout=5
)
print(result2.stdout if result2.stdout else "No flag parsing in extension code")
EOFRepository: openshift/must-gather-operator
Length of output: 240
🏁 Script executed:
# Check the actual Cobra integration - does the root command link k8s flags?
rg "cmd\.Flags\(\)\.AddGoFlagSet|root\.Flags\(\)\.AddGoFlagSet|PersistentFlags\(\)\.AddGoFlagSet" cmd/extension/ -B 5 -A 5Repository: openshift/must-gather-operator
Length of output: 56
Move AfterReadingAllFlags into a PersistentPreRun hook to ensure it runs after Cobra parses CLI flags.
Currently, AfterReadingAllFlags is called at line 30 before root.Execute(), so TestContext is finalized before Cobra has a chance to parse and apply any command-line overrides for kubeconfig, provider, and other framework settings.
However, there is a more fundamental issue: the k8s framework flags registered via util.InitStandardFlags() (line 29) are never actually parsed because they are registered to flag.CommandLine, but the Cobra command does not integrate them. The standard library's flag.Parse() is never called, nor does the root Cobra command call AddGoFlagSet(flag.CommandLine). This means kubeconfig and provider flags cannot be overridden from the CLI at all in the current setup.
The fix requires both:
- Move
AfterReadingAllFlagstoPersistentPreRunso it runs after Cobra parses flags - Integrate the k8s framework flags into the Cobra command by adding
flag.CommandLineto the root command's flag set
Suggested approach
func main() {
// Initialize test framework flags (required for kubeconfig, provider, etc.)
util.InitStandardFlags()
- framework.AfterReadingAllFlags(&framework.TestContext)
logs.InitLogs()
defer logs.FlushLogs()
...
root := &cobra.Command{
Long: "Must-gather-operator Tests",
+ PersistentPreRun: func(cmd *cobra.Command, args []string) {
+ framework.AfterReadingAllFlags(&framework.TestContext)
+ },
}
+ root.PersistentFlags().AddGoFlagSet(flag.CommandLine)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/extension/main.go` around lines 28 - 30, Move the framework finalization
into a Cobra PersistentPreRun and integrate the stdlib flags into the root
command: call util.InitStandardFlags() as before, then add flag.CommandLine to
the root Cobra flags using
rootCmd.PersistentFlags().AddGoFlagSet(flag.CommandLine) so the k8s framework
flags get parsed by Cobra, and relocate the
framework.AfterReadingAllFlags(&framework.TestContext) call into
rootCmd.PersistentPreRun (or append it to any existing PersistentPreRun chain)
so AfterReadingAllFlags runs after Cobra has parsed CLI overrides (instead of
calling it before root.Execute()).
Comment on lines
+107
to
+149
| func registerSuites(ext *e.Extension) { | ||
| suites := []e.Suite{ | ||
| { | ||
| Name: "must-gather-operator/conformance/parallel", | ||
| Parents: []string{ | ||
| "openshift/conformance/parallel", | ||
| }, | ||
| Description: "Parallel conformance tests (Level0, non-serial, non-disruptive)", | ||
| Qualifiers: []string{ | ||
| `name.contains("[Level0]") && !(name.contains("[Serial]") || name.contains("[Disruptive]"))`, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "must-gather-operator/conformance/serial", | ||
| Parents: []string{ | ||
| "openshift/conformance/serial", | ||
| }, | ||
| Description: "Serial conformance tests (must run sequentially)", | ||
| Qualifiers: []string{ | ||
| `name.contains("[Level0]") && name.contains("[Serial]") && !name.contains("[Disruptive]")`, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "must-gather-operator/disruptive", | ||
| Parents: []string{"openshift/disruptive"}, | ||
| Description: "Disruptive tests (may affect cluster state)", | ||
| Qualifiers: []string{ | ||
| `name.contains("[Disruptive]")`, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "must-gather-operator/non-disruptive", | ||
| Description: "All non-disruptive tests (safe for development clusters)", | ||
| Qualifiers: []string{ | ||
| `!name.contains("[Disruptive]")`, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "must-gather-operator/all", | ||
| Description: "All must-gather-operator tests", | ||
| // No qualifiers means all tests from this extension will be included | ||
| }, | ||
| } |
There was a problem hiding this comment.
These suite selectors don’t catch the new specs.
The qualifiers depend on [Level0], [Serial], and [Disruptive], but the specs added in test/e2e/extension/mustgather.go don’t carry those tokens. As written, they only land in non-disruptive/all, not the conformance/disruptive suites introduced here.
| github.com/onsi/gomega v1.36.1 | ||
| github.com/openshift/api v0.0.0-20240522145529-93d6bda14341 | ||
| github.com/go-logr/logr v1.4.3 | ||
| github.com/onsi/ginkgo/v2 v2.25.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the exact require/replace entries and surface callsites likely to rely on newer Ginkgo APIs.
set -euo pipefail
echo "== ginkgo require/replace entries =="
rg -n '^\s*github.com/onsi/ginkgo/v2\s+v' go.mod -n -C1
rg -n '^\s*github.com/onsi/ginkgo/v2\s+=>\s+github.com/openshift/onsi-ginkgo/v2\s+v' go.mod -n -C1
echo
echo "== ginkgo usage in extension/e2e paths (spot-check API surface) =="
rg -n --type=go -C2 '\b(ginkgo|Describe|Context|When|It|Before(All|Each)|After(All|Each)|DeferCleanup|Label|Serial|Ordered)\b' test cmdRepository: openshift/must-gather-operator
Length of output: 50386
Verify version compatibility between pinned and replaced Ginkgo versions before merge.
Line 7 pins github.com/onsi/ginkgo/v2 at v2.25.1, but line 335 replaces it with an OpenShift fork at v2.6.1-0.20241205171354-8006f302fd12. This ~19 minor version gap introduces risk:
- Newer API features in v2.25.1 may not exist in the fork.
- Bug fixes and performance improvements are absent from the older fork version.
- Test code extensively uses Ginkgo v2 APIs (Describe, Context, BeforeEach, BeforeAll, It, By, Ordered, etc.) across test/e2e/ and test/ directories—these must remain compatible with the fork version.
Confirm this replace is intentional and that all APIs currently in use are supported by the fork before proceeding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 7, go.mod pins github.com/onsi/ginkgo/v2 at v2.25.1 but a
replace directive targets an OpenShift fork at v2.6.1-0... which may be
incompatible; verify intent and compatibility by confirming the replace is
deliberate, auditing all uses of Ginkgo v2 APIs (Describe, Context, BeforeEach,
BeforeAll, It, By, Ordered, etc.) across test/e2e and test/ directories for APIs
or behaviors introduced after v2.6.1, and either update the replace to a
fork/version that provides the same or newer public API as v2.25.1 or change the
pinned module version to match the replaced fork; if the replace must remain,
add a short compatibility note in go.mod or repository docs explaining why the
older fork is acceptable and list any tested Ginkgo features to avoid surprises.
| k8s.io/client-go v0.34.1 | ||
| k8s.io/component-base v0.34.1 | ||
| k8s.io/kube-openapi v0.0.0-20250814151709-d7b6acb124c3 | ||
| k8s.io/kubernetes v1.34.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify k8s core module version alignment in go.mod and related replace targets.
set -euo pipefail
echo "== k8s.io/kubernetes require =="
rg -n '^\s*k8s\.io/kubernetes\s+v' go.mod -n -C1
echo
echo "== k8s.io/kubernetes replace =="
rg -n '^\s*k8s\.io/kubernetes\s+=>\s+github\.com/openshift/kubernetes\s+v' go.mod -n -C1
echo
echo "== top-level k8s.io/* direct requires =="
rg -n '^\s*k8s\.io/(api|apimachinery|client-go|component-base|kubernetes)\s+v' go.mod -nRepository: openshift/must-gather-operator
Length of output: 820
Reconcile k8s.io/kubernetes version mismatch between require and replace.
Line 27 requires k8s.io/kubernetes v1.34.1, but Line 360 replaces it with github.com/openshift/kubernetes v1.30.1-0.20251017123720-96593f323733. This 4-version gap is inconsistent with all other k8s.io modules pinned to v0.34.1 (v1.34.1), and will likely cause transitive API compatibility issues across the module set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 27, The go.mod has a version mismatch: the require line
specifies k8s.io/kubernetes v1.34.1 while the replace maps it to
github.com/openshift/kubernetes v1.30.1-0.20251017123720-96593f323733; update
the replace entry to use a compatible v1.34.1 OpenShift fork (or remove the
replace) so the required module and the replacement are aligned with the rest of
the k8s.io modules (ensure the replace target version matches v1.34.1 or drop
the replace and vendor a compatible module), adjusting the replace statement
referencing github.com/openshift/kubernetes accordingly.
Comment on lines
+81
to
+82
| if !strings.Contains(output, "httpsProxy") && !strings.Contains(output, "httpProxy") { | ||
| g.Skip("Skip for non-proxy cluster - this test requires a proxy-enabled environment") |
There was a problem hiding this comment.
Allow proxy configs that only set httpsProxy.
This test gates on either proxy being present, but later requires httpProxy and compares both env vars unconditionally. A valid HTTPS-only cluster proxy config will fail here.
Also applies to: 98-103, 118-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/mustgather.go` around lines 81 - 82, The current gate
requires both proxies but later unconditionally compares httpProxy which breaks
HTTPS-only clusters; update the initial check to skip only when neither proxy is
present (use OR: check if output contains "httpsProxy" || "httpProxy"), and
modify the later assertions (around the blocks referenced at lines 98-103 and
118-120) to only compare httpProxy-related env vars when "httpProxy" is present
in output (similarly handle httpsProxy checks independently) so each proxy is
validated only if configured.
Comment on lines
+377
to
+385
| cmd := fmt.Sprintf("test -d /data/%s && echo 'exists' || echo 'not exists'", subPath) | ||
| output, err := oc.AsAdmin().Run("exec").Args(readerPodName, "-n", namespace, "-c", "reader-container", "--", "sh", "-c", cmd).Output() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to check PVC data: %v", err) | ||
| } | ||
|
|
||
| if !strings.Contains(output, "exists") { | ||
| return fmt.Errorf("PVC does not contain expected data at path: %s", subPath) | ||
| } |
There was a problem hiding this comment.
These negative probes still satisfy the "exists" check.
The shell branches emit not exists / not-exists, and both still make strings.Contains(..., "exists") return true. Missing PVC data or a missing audit_logs directory will therefore pass as success.
Possible fix
- if !strings.Contains(output, "exists") {
+ if strings.TrimSpace(output) != "exists" {
return fmt.Errorf("PVC does not contain expected data at path: %s", subPath)
}
@@
- if err == nil && strings.Contains(output, "exists") {
+ if err == nil && strings.TrimSpace(output) == "exists" {
directoryFound = true
e2e.Logf("Found audit_logs directory at /data/%s/audit_logs", subPath)
} else {Also applies to: 552-558
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/supportloggather_utils.go` around lines 377 - 385, The
check using strings.Contains(output, "exists") wrongly matches "not exists";
update the probe logic in the block that builds cmd and reads output (the
oc.AsAdmin().Run(...).Output() call using readerPodName and namespace) to detect
existence precisely—e.g. trim whitespace and compare equality
(strings.TrimSpace(output) == "exists") or test for the negative case first
(strings.Contains(output, "not exists") -> failure) so that only an exact
"exists" result is treated as success; apply the same change to the similar
block around lines 552-558.
| metadata: | ||
| name: "${NAME}" | ||
| spec: | ||
| refreshInterval: "${REFREASHINTERVAL}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'REFREASHINTERVAL|REFRESHINTERVAL'Repository: openshift/must-gather-operator
Length of output: 21390
🏁 Script executed:
head -20 test/e2e/extension/testdata/bindata.goRepository: openshift/must-gather-operator
Length of output: 1165
🏁 Script executed:
cat -n test/e2e/extension/testdata/eso/externalsecret-gcpsm-version.yamlRepository: openshift/must-gather-operator
Length of output: 1434
Fix typo in template parameter name across test fixtures.
REFREASHINTERVAL is misspelled (should be REFRESHINTERVAL). While the typo is internally consistent and doesn't cause runtime failures, it degrades readability and maintainability. This typo appears in multiple test fixture files.
Proposed change
- refreshInterval: "${REFREASHINTERVAL}"
+ refreshInterval: "${REFRESHINTERVAL}"
@@
- - name: REFREASHINTERVAL
+ - name: REFRESHINTERVAL
value: "1m"This change applies to: test/e2e/extension/testdata/eso/externalsecret-gcpsm-version.yaml (lines 11, 30-31) and other test fixture files with the same pattern (externalsecret-.yaml, pushsecret-.yaml).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/testdata/eso/externalsecret-gcpsm-version.yaml` at line
11, Replace the misspelled template parameter REFREASHINTERVAL with
REFRESHINTERVAL across all test fixtures (e.g., in externalsecret-*.yaml and
pushsecret-*.yaml templates) so placeholders and any test wiring use the correct
name; search for occurrences of "REFREASHINTERVAL" in the testdata and update
them to "REFRESHINTERVAL", and also update any tests or helper code that set or
reference that env/template variable to use REFRESHINTERVAL to keep names
consistent.
Comment on lines
+91
to
+100
| func ListFixtures() []string { | ||
| names := AssetNames() | ||
| fixtures := make([]string, 0, len(names)) | ||
| for _, name := range names { | ||
| if strings.HasPrefix(name, "testdata/") { | ||
| fixtures = append(fixtures, strings.TrimPrefix(name, "testdata/")) | ||
| } | ||
| } | ||
| sort.Strings(fixtures) | ||
| return fixtures |
There was a problem hiding this comment.
ListFixtures() filters out every embedded asset.
AssetNames() here returns names like mustgather/mustgather.yaml, not testdata/..., so this loop currently always returns an empty slice. Filter on the actual embedded prefixes/suffixes instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/testdata/fixtures.go` around lines 91 - 100, ListFixtures
currently filters only names starting with "testdata/" but AssetNames yields
entries like "mustgather/mustgather.yaml", so the loop returns empty; update
ListFixtures to not require the "testdata/" prefix — iterate all names returned
by AssetNames(), append strings.TrimPrefix(name, "testdata/") (so names without
that prefix are unchanged), then sort and return; keep the function name
ListFixtures and call to AssetNames unchanged.
Comment on lines
+5
to
+23
| spec: | ||
| containers: | ||
| - image: registry.k8s.io/e2e-test-images/busybox:1.29-4 | ||
| name: busybox | ||
| imagePullPolicy: IfNotPresent | ||
| command: | ||
| - "/bin/sleep" | ||
| - "10000" | ||
| volumeMounts: | ||
| - name: secrets-store-inline | ||
| mountPath: "/mnt/secrets-store" | ||
| readOnly: true | ||
| volumes: | ||
| - name: secrets-store-inline | ||
| csi: | ||
| driver: secrets-store.csi.k8s.io | ||
| readOnly: true | ||
| volumeAttributes: | ||
| secretProviderClass: "vault-foo" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/e2e/extension/testdata/sscsi/73739/spc_vault_pod.yaml"
echo "Inspecting security-related fields in ${file}..."
rg -n 'securityContext|allowPrivilegeEscalation|readOnlyRootFilesystem|runAsNonRoot|seccompProfile|capabilities' "$file" || trueRepository: openshift/must-gather-operator
Length of output: 173
Harden pod/container security context to avoid policy rejections.
This fixture lacks security context configuration. On restricted/hardened clusters, the default security posture will cause admission failures and break e2e reliability.
Suggested hardening patch
spec:
+ securityContext:
+ seccompProfile:
+ type: RuntimeDefault
containers:
- image: registry.k8s.io/e2e-test-images/busybox:1.29-4
name: busybox
imagePullPolicy: IfNotPresent
+ securityContext:
+ allowPrivilegeEscalation: false
+ readOnlyRootFilesystem: true
+ runAsNonRoot: true
+ capabilities:
+ drop:
+ - ALL
command:
- "/bin/sleep"
- "10000"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| spec: | |
| containers: | |
| - image: registry.k8s.io/e2e-test-images/busybox:1.29-4 | |
| name: busybox | |
| imagePullPolicy: IfNotPresent | |
| command: | |
| - "/bin/sleep" | |
| - "10000" | |
| volumeMounts: | |
| - name: secrets-store-inline | |
| mountPath: "/mnt/secrets-store" | |
| readOnly: true | |
| volumes: | |
| - name: secrets-store-inline | |
| csi: | |
| driver: secrets-store.csi.k8s.io | |
| readOnly: true | |
| volumeAttributes: | |
| secretProviderClass: "vault-foo" | |
| spec: | |
| securityContext: | |
| seccompProfile: | |
| type: RuntimeDefault | |
| containers: | |
| - image: registry.k8s.io/e2e-test-images/busybox:1.29-4 | |
| name: busybox | |
| imagePullPolicy: IfNotPresent | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| readOnlyRootFilesystem: true | |
| runAsNonRoot: true | |
| capabilities: | |
| drop: | |
| - ALL | |
| command: | |
| - "/bin/sleep" | |
| - "10000" | |
| volumeMounts: | |
| - name: secrets-store-inline | |
| mountPath: "/mnt/secrets-store" | |
| readOnly: true | |
| volumes: | |
| - name: secrets-store-inline | |
| csi: | |
| driver: secrets-store.csi.k8s.io | |
| readOnly: true | |
| volumeAttributes: | |
| secretProviderClass: "vault-foo" |
🧰 Tools
🪛 Checkov (3.2.510)
[medium] 1-23: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-23: Minimize the admission of root containers
(CKV_K8S_23)
🪛 Trivy (0.69.3)
[error] 7-16: Root file system is not read-only
Container 'busybox' of Pod 'secrets-store-inline' should set 'securityContext.readOnlyRootFilesystem' to true
Rule: KSV-0014
(IaC/Kubernetes)
[error] 7-16: Default security context configured
container secrets-store-inline in default namespace is using the default security context
Rule: KSV-0118
(IaC/Kubernetes)
[error] 5-23: Default security context configured
pod secrets-store-inline in default namespace is using the default security context, which allows root privileges
Rule: KSV-0118
(IaC/Kubernetes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/testdata/sscsi/73739/spc_vault_pod.yaml` around lines 5 -
23, The pod spec for the fixture (container name "busybox" and volume
"secrets-store-inline" using CSI driver "secrets-store.csi.k8s.io") is missing
hardened security contexts; add a pod-level securityContext and a
container.securityContext for "busybox" that enforces runAsNonRoot: true and a
non-zero runAsUser (e.g. 1000), sets allowPrivilegeEscalation: false, drops all
capabilities, sets readOnlyRootFilesystem: true, and applies a seccomp/profile
of "RuntimeDefault" (or appropriate restricted profile) so the pod passes
restricted/hardened admission policies. Ensure these fields are added alongside
the existing spec/container definitions (pod-level securityContext for fsGroup
or supplementalGroups as needed and container-level securityContext for the
busybox container).
Comment on lines
+117
to
+129
| for bIdx := range bindings { | ||
| if bindings[bIdx].Role != role { | ||
| continue | ||
| } | ||
| bindingIndex = bIdx | ||
| for mIdx := range bindings[bindingIndex].Members { | ||
| if bindings[bindingIndex].Members[mIdx] != member { | ||
| continue | ||
| } | ||
| memberIndex = mIdx | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Logic bug: outer loop doesn't break after finding the role.
If the same role appears in multiple bindings, this loop will only process the last one, potentially missing the member or modifying the wrong binding.
for bIdx := range bindings {
if bindings[bIdx].Role != role {
continue
}
bindingIndex = bIdx
for mIdx := range bindings[bindingIndex].Members {
if bindings[bindingIndex].Members[mIdx] != member {
continue
}
memberIndex = mIdx
break
}
+ if memberIndex != -1 {
+ break
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/utils.go` around lines 117 - 129, The outer loop over
bindings can overwrite bindingIndex when the same role appears multiple times;
after you find the matching role and search its Members for the member, stop
iterating further to avoid picking a later duplicate role. Modify the loop that
iterates over bindings (the code using bindings[bIdx], role, bindingIndex, and
memberIndex) to break out of the outer loop once you set bindingIndex and (after
the inner member search) set memberIndex — e.g. perform the inner member search,
then break the outer loop (or use a labeled break/return) so later bindings with
the same role cannot overwrite the indices.
Migrates must-gather e2e tests from openshift-tests-private to OTE framework using monorepo strategy. Tests are placed in test/e2e/extension/ to avoid conflicts with existing test/e2e/ structure. Changes: - Add cmd/extension/main.go: OTE extension entry point that filters and registers tests from test/e2e/extension/ directory - Add test/e2e/extension/mustgather.go: 7 must-gather tests ([OTP] annotated) - Add test/e2e/extension/supportloggather_utils.go: must-gather helper functions - Add test/e2e/extension/olm_utils.go: OLM installation utilities - Add test/e2e/extension/utils.go: minimal utility functions (getRandomString, IsDeploymentReady, dumpResource) - Add test/e2e/extension/testdata/mustgather/: test fixtures for must-gather tests - Add bindata.mk: Makefile for generating bindata from testdata files - Update Dockerfile.openshift: multi-stage build for test extension binary - Update Makefile: add tests-ext-build target - Update go.mod: add OTE dependencies and replace directives - Update go.sum: dependency checksums Only must-gather test fixtures are included (testdata/mustgather/). ESO and SSCSI test data are not migrated as those tests are not included. The test extension binary can be built with 'make tests-ext-build' and will be packaged as must-gather-operator-test-extension.tar.gz in the container image for CI execution. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
@KeenonLee: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.