ESO-566: Allow overriding operand container args outside the ExternalSecretsConfig API - #174
Merged
Merged
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references ESO-566 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 "5.0.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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe operator now supports environment-based argument overrides for four operand Deployments. It parses and merges ChangesOperand argument overrides
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant E2ESuite
participant ManagerEnvironment
participant DeploymentGenerator
participant OperandDeployment
participant ExternalSecretsConfig
E2ESuite->>ManagerEnvironment: Set OPERAND_*_ARGS
ManagerEnvironment->>DeploymentGenerator: Apply manager environment
DeploymentGenerator->>OperandDeployment: Generate updated arguments
E2ESuite->>OperandDeployment: Verify arguments and readiness
E2ESuite->>ExternalSecretsConfig: Check Ready or Degraded condition
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pkg/controller/external_secrets/deployments.go (1)
843-872: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning a clone for the empty-override path.
mergeContainerArgsreturnsbasedirectly whenoverridesis empty, but returns a clone otherwise. The inconsistent aliasing is harmless today because the caller assigns the result back to the same field. A clone in both paths removes the aliasing surprise for future callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/external_secrets/deployments.go` around lines 843 - 872, Update mergeContainerArgs so it returns a clone of base when overrides is empty, matching the cloned result used for non-empty overrides and avoiding direct slice aliasing.test/e2e/helpers_test.go (1)
438-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSort the appended env var names to keep the merge deterministic.
The loop at Line 457 iterates a Go map, so the append order of new env vars varies between calls. Each call to
setOperatorManagerEnvwith the same fourOPERAND_*_ARGSvalues can therefore produce a differentEnvslice order. That order change is a spec diff, so it triggers an extra operator rollout and lengthens the wait inwaitForOperatorManagerEnv.Also, Line 452 duplicates the assignment on Line 455 and can be removed.
♻️ Proposed fix for deterministic ordering
for _, env := range existing { if _, drop := remove[env.Name]; drop { continue } if val, ok := set[env.Name]; ok { env.Value = val env.ValueFrom = nil - seen[env.Name] = true } out = append(out, env) seen[env.Name] = true } - for name, val := range set { - if seen[name] { - continue - } - out = append(out, corev1.EnvVar{Name: name, Value: val}) - } + names := make([]string, 0, len(set)) + for name := range set { + if !seen[name] { + names = append(names, name) + } + } + sort.Strings(names) + for _, name := range names { + out = append(out, corev1.EnvVar{Name: name, Value: set[name]}) + } return outAdd
"sort"to the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/helpers_test.go` around lines 438 - 464, Update mergeEnvVars to sort the keys from set before appending new corev1.EnvVar entries, preserving deterministic Env slice ordering across calls; add the required sort import. Remove the redundant seen[env.Name] assignment inside the set-update branch, keeping the assignment after the branch.test/e2e/operand_args_test.go (1)
190-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the coupling between the positional-override spec and the recovery spec.
The spec at Line 190 leaves
OPERAND_WEBHOOK_ARGS="webhook,--port=10251"set. The spec at Line 208 depends on that Degraded state. TheOrdereddecorator preserves the order, but a label filter or--focusthat selects only the recovery spec makes its first assertion at Line 211 fail. Consider moving the invalid-args setup into the recovery spec itself so the spec is self-contained.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/operand_args_test.go` around lines 190 - 224, Make the recovery spec “should recover from Degraded when invalid OPERAND_*_ARGS are corrected” self-contained by applying the invalid webhook positional override within that spec before asserting Degraded. Do not rely on the preceding positional-override spec or execution order; retain the existing correction, readiness wait, and valid-override verification flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/external-secrets-operator/go.mod`:
- Around line 32-48: Upgrade github.com/google/cel-go from v0.27.0 to v0.29.0 in
both dependency graphs, regenerate dependent artifacts, and preserve checksum
validation without changes. Update affected entries in
cmd/external-secrets-operator/go.mod lines 32-48, 80, and 89-90; go.mod lines
28-43; test/go.mod lines 12-14, 36-52, and 73; and tools/go.mod lines 90-103,
129-130, 215, 260, and 270-271. Document the license review for
github.com/operator-framework/api v0.42.0 and ensure releases generate SBOMs,
provenance attestations, and signed artifacts.
In `@test/e2e/helpers_test.go`:
- Around line 348-360: Update the CSV listing error handling in findOperatorCSV
to also tolerate Kubernetes API no-match errors by checking
meta.IsNoMatchError(err) alongside k8serrors.IsNotFound(err). Add the required
api/meta import, while preserving propagation of all other list errors so
non-OLM clusters can fall back to the Deployment path.
In `@test/e2e/operand_args_test.go`:
- Around line 94-95: Update the suite cleanup around ensureBitwardenOperandReady
so the Bitwarden plugin enabled during BeforeAll is disabled and its Deployment
is removed in AfterAll, restoring the shared ExternalSecretsConfig for later
suites. Ensure the cleanup covers the setup at the referenced provisioning block
and the related setup lines 104-115, while preserving existing operator
environment-variable cleanup.
In `@test/e2e/README.md`:
- Line 140: Move the OverrideOperandArgs row out of the e2e_test.go table in the
README, then add a separate operand_args_test.go heading and table containing
that row, matching the structure used for trusted_ca_bundle_test.go.
---
Nitpick comments:
In `@pkg/controller/external_secrets/deployments.go`:
- Around line 843-872: Update mergeContainerArgs so it returns a clone of base
when overrides is empty, matching the cloned result used for non-empty overrides
and avoiding direct slice aliasing.
In `@test/e2e/helpers_test.go`:
- Around line 438-464: Update mergeEnvVars to sort the keys from set before
appending new corev1.EnvVar entries, preserving deterministic Env slice ordering
across calls; add the required sort import. Remove the redundant seen[env.Name]
assignment inside the set-update branch, keeping the assignment after the
branch.
In `@test/e2e/operand_args_test.go`:
- Around line 190-224: Make the recovery spec “should recover from Degraded when
invalid OPERAND_*_ARGS are corrected” self-contained by applying the invalid
webhook positional override within that spec before asserting Degraded. Do not
rely on the preceding positional-override spec or execution order; retain the
existing correction, readiness wait, and valid-override verification flow.
🪄 Autofix
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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8eab44f7-e466-46c5-a438-584b33fdf6ca
⛔ Files ignored due to path filters (202)
cmd/external-secrets-operator/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtest/go.sumis excluded by!**/*.sumtools/go.sumis excluded by!**/*.sumvendor/github.com/go-openapi/jsonpointer/.cliff.tomlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/CONTRIBUTORS.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/SECURITY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonpointer/pointer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonreference/.cliff.tomlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonreference/.editorconfigis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonreference/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonreference/CONTRIBUTORS.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonreference/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonreference/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonreference/SECURITY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonreference/internal/normalize_url.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/jsonreference/reference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/.codecov.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/.mockery.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/SECURITY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/cmdutils/cmd_utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/cmdutils/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/cmdutils_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/conv/convert.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/conv/convert_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/conv/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/conv/format.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/conv/sizeof.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/conv/type_constraints.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/conv_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/fileutils/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/fileutils/file.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/fileutils/path.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/fileutils_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/go.workis excluded by!**/*.work,!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/go.work.sumis excluded by!**/*.sum,!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonname/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonname/name_provider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonname_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/ifaces/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/ifaces/ifaces.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/ifaces/registry_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/registry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/stdlib/json/adapter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/stdlib/json/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/stdlib/json/lexer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/stdlib/json/ordered_map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/stdlib/json/pool.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/stdlib/json/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/adapters/stdlib/json/writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/concat.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils/ordered_map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/jsonutils_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/loading/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/loading/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/loading/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/loading/loading.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/loading/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/loading/yaml.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/loading_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling/initialism_index.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling/name_lexem.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling/name_mangler.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling/pools.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling/split.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling/string_bytes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/mangling_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/netutils/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/netutils/net.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/netutils_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/stringutils/collection_formats.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/stringutils/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/stringutils/strings.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/stringutils_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/typeutils/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/typeutils/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/typeutils_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/yamlutils/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/yamlutils/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/yamlutils/ordered_map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/yamlutils/yaml.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-openapi/swag/yamlutils_iface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/cel/BUILD.bazelis excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/cel/env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/cel/folding.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/cel/library.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/cel/optimizer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/cel/program.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/cel/templates/authoring.tmplis excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/cel/validator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/checker/checker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/checker/env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/checker/scopes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/ast/ast.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/debug/debug.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/env/BUILD.bazelis excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/env/env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/BUILD.bazelis excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/bool.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/bytes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/double.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/duration.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/int.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/json_value.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/list.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/null.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/object.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/pb/type.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/timestamp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/common/types/uint.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/ext/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/ext/bindings.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/ext/comprehensions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/ext/extension_option_factory.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/ext/formatting_v2.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/ext/native.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/ext/regex.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/interpreter/attribute_patterns.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/interpreter/attributes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/interpreter/interpretable.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/interpreter/interpreter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/interpreter/planner.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/cel-go/parser/helper.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/gnostic-models/extensions/extension.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/gnostic-models/openapiv2/OpenAPIv2.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/gnostic-models/openapiv3/OpenAPIv3.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/gnostic-models/openapiv3/annotations.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/josharian/intern/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/josharian/intern/intern.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/josharian/intern/license.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/buffer/pool.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/jlexer/bytestostr.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/jlexer/bytestostr_nounsafe.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/jlexer/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/jlexer/lexer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/jwriter/writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/raw.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mailru/easyjson/unknown_fields.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/lib/release/release.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/lib/version/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/lib/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/lib/version/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/operator-framework/api/pkg/operators/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/operators/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/operators/v1alpha1/catalogsource_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/operators/v1alpha1/clusterserviceversion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/operators/v1alpha1/clusterserviceversion_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/operators/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/operators/v1alpha1/installplan_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/operators/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/operators/v1alpha1/subscription_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/api/pkg/operators/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/operator-framework/api/pkg/operators/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/sirupsen/logrus/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/appveyor.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/entry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/hooks.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/logger.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/logrus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/terminal_check_bsd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/terminal_check_unix.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/terminal_check_wasi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/terminal_check_wasip1.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/sirupsen/logrus/text_formatter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stoewer/go-strcase/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/stoewer/go-strcase/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/stoewer/go-strcase/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/stoewer/go-strcase/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/stoewer/go-strcase/camel.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stoewer/go-strcase/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stoewer/go-strcase/helper.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stoewer/go-strcase/kebab.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stoewer/go-strcase/snake.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (12)
cmd/external-secrets-operator/go.modgo.modpkg/controller/external_secrets/constants.gopkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/deployments_test.gotest/e2e/README.mdtest/e2e/e2e_suite_test.gotest/e2e/helpers_test.gotest/e2e/operand_args_test.gotest/e2e/trusted_ca_bundle_test.gotest/go.modtools/go.mod
… typed OLM e2e helpers Signed-off-by: Bharath B <bhb@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/external_secrets/deployments.go`:
- Around line 852-857: Update the custom argument validation in the surrounding
mergeContainerArgs logic to reject tokens whose parsed flag key from argFlagKey
is exactly "--", including "--=value", before appending the override. Preserve
the existing UserConfigurationError path and add a test covering the invalid
"--=value" input and resulting ExternalSecretsConfig degradation.
🪄 Autofix
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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9eec041a-409d-4dbe-ab37-aca17b701ec3
⛔ Files ignored due to path filters (1)
tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
pkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/deployments_test.gotest/e2e/operand_args_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/operand_args_test.go
- pkg/controller/external_secrets/deployments_test.go
…SecretsConfig API Signed-off-by: Bharath B <bhb@redhat.com>
Contributor
|
/lgtm |
Contributor
|
Scheduling required tests: |
Contributor
Author
|
adding below labels based on the CI results |
openshift-ci
Bot
added
docs-approved
|
@bharath-b-rh: all tests passed! 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #174 +/- ##
==========================================
- Coverage 46.26% 45.00% -1.27%
==========================================
Files 31 31
Lines 5464 5557 +93
==========================================
- Hits 2528 2501 -27
- Misses 2628 2739 +111
- Partials 308 317 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
4 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.
Summary
--key=valueflags:OPERAND_EXTERNAL_SECRETS_ARGSOPERAND_WEBHOOK_ARGSOPERAND_CERT_CONTROLLER_ARGSOPERAND_BITWARDEN_SDK_SERVER_ARGS--) overrides are rejected asUserConfigurationErrorand surface as ExternalSecretsConfigDegraded.advancedOverridesis the migration path).E2E / test helpers
getDeploymentObjectwiring.Feature:OverrideOperandArgssets env through OLMSubscription.spec.config.env(typed OLM APIs), waits until the Ready manager pod has the env, then verifies operand Deployments (including Degraded + recovery).setOperatorManagerEnv/unsetOperatorManagerEnvhelpers are reusable for other manager env vars (OPERATOR_LOG_LEVEL,METRICS_*, etc.).Test plan
go test ./pkg/controller/external_secrets/ -count=1make test-e2e E2E_GINKGO_LABEL_FILTER="Feature:OverrideOperandArgs"Related to #154
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests