feat: add extraArgs field to ComponentConfig to allow tuning controller arguments#155
Open
joyrex2001 wants to merge 2 commits into
Open
feat: add extraArgs field to ComponentConfig to allow tuning controller arguments#155joyrex2001 wants to merge 2 commits into
joyrex2001 wants to merge 2 commits into
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an ChangesExtraArgs support for ComponentConfig
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: joyrex2001 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @joyrex2001. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_test.go`:
- Around line 1957-1967: The test case "both extra args and override env applied
together" specifies both ExtraArgs and OverrideEnv in the componentConfig but
only verifies the expectedArgs assertions. Add verification after the existing
args assertions to confirm that the OverrideEnv values are correctly applied to
the deployment, specifically checking that the LOG_LEVEL environment variable
with value "debug" is present in the container environment of the
external-secrets container. This ensures that regressions in OverrideEnv
handling will be caught by this combined test case.
- Around line 671-688: The validateDeployment function has a logic flaw where it
returns early upon finding "--concurrent=5", allowing the test to pass even if
"--concurrent=1" still exists later in the args list. Refactor the assertion
logic to scan through all args in the "external-secrets" container, tracking
whether both "--concurrent=1" and "--concurrent=5" are present. After completing
the full iteration through all args, assert that "--concurrent=1" does not exist
and that "--concurrent=5" is present, instead of returning early when
"--concurrent=5" is first found.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 39f4bd85-f393-4207-997c-36090f30eb9d
⛔ Files ignored due to path filters (1)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (11)
api/v1alpha1/external_secrets_config_types.gobundle.Dockerfilebundle/manifests/external-secrets.io_externalsecrets.yamlbundle/manifests/openshift-external-secrets-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlbundle/metadata/annotations.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamlconfig/manifests/bases/openshift-external-secrets-operator.clusterserviceversion.yamldocs/api_reference.mdpkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/deployments_test.go
💤 Files with no reviewable changes (1)
- bundle/manifests/external-secrets.io_externalsecrets.yaml
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
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
`@bundle/manifests/openshift-external-secrets-operator.clusterserviceversion.yaml`:
- Around line 223-231: The createdAt field in the YAML contains unresolved Git
merge conflict markers (<<<<<<, ======, >>>>>>> symbols) that make the file
invalid YAML. Since
bundle/manifests/openshift-external-secrets-operator.clusterserviceversion.yaml
is a generated file, do not manually edit these conflict markers. Instead,
resolve the merge conflict in the upstream source manifest file that generates
this bundle artifact, then regenerate the entire bundle/ directory using the
appropriate generation tool or command to ensure all artifacts are consistent
and the conflict markers are eliminated.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cdeaf5a4-b0d4-44c5-b691-4322e8137a9e
⛔ Files ignored due to path filters (1)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (7)
api/v1alpha1/external_secrets_config_types.gobundle/manifests/openshift-external-secrets-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_externalsecretsconfigs.yamlconfig/crd/bases/operator.openshift.io_externalsecretsconfigs.yamldocs/api_reference.mdpkg/controller/external_secrets/deployments.gopkg/controller/external_secrets/deployments_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- api/v1alpha1/external_secrets_config_types.go
- bundle/manifests/operator.openshift.io_externalsecretsconfigs.yaml
- config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml
- pkg/controller/external_secrets/deployments.go
- docs/api_reference.md
- pkg/controller/external_secrets/deployments_test.go
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
1 participant
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
Fixes #154
Adds an extraArgs field to ComponentConfig so users can supply additional command-line arguments to any external-secrets component container without forking the operator. User-supplied arguments are merged on top of the operator's defaults, with user values taking precedence when the same flag is already present.
Usage
Supplying a flag that is already set by the operator (e.g.
--concurrent) overrides the operator default. New flags are appended. The field is available on all four components:ExternalSecretsCoreController,Webhook,CertController, andBitwardenSDKServer.Changes
API (
api/v1alpha1/external_secrets_config_types.go)ExtraArgs []stringfield toComponentConfigwith a max of 50 items and+listType=atomic.Controller (
pkg/controller/external_secrets/deployments.go)mergeArgs(container, extraArgs)— merges extra args onto a container's existing arg list, matching flags by the key portion (before =) so that--concurrent=20correctly replaces--concurrent=1.mergeArgsintoapplyUserDeploymentConfigsafter the existingOverrideEnvblock, applying only to the target component container.Generated files (make update)
api/v1alpha1/zz_generated.deepcopy.go—ComponentConfig.DeepCopyIntoupdated to deep-copyExtraArgs.config/crd/bases/operator.openshift.io_externalsecretsconfigs.yaml— CRD schema updated with extraArgs field.bundle/anddocs/refreshed accordingly.Tests
TestMergeArgs— unit tests for the merge function covering nil init, flag-key override, append, mixed, empty, and positional args.TestApplyUserDeploymentConfigsWithExtraArgs— table-driven tests verifying correct behaviour for all four component containers, empty args no-op, combined withOverrideEnv, and init-container isolation.TestCreateOrApplyDeploymentsexercising the fullcreateOrApplyDeploymentscall chain: one appending a new flag (--client-qps=100), one overriding an operator default (--concurrent=1→--concurrent=5).Summary by CodeRabbit
New Features
extraArgs(spec.controllerConfig.componentConfigs[].deploymentConfigs.extraArgs), limited to 50 items.extraArgsare merged into the operator’s default component arguments using key-based override semantics (key=value) with de-duplication, applied only to the targeted component container.Documentation
extraArgsbehavior and limits.Tests
OverrideEnv).