openshift
feat: Allows for override replicas quantity to deploy a HA cert manager
As there was IMHO no way to deploy a HA cert manager, via the cert-manager-operator, I've added the option to override the replicas via the resource. If no override is done, the default (1) replicas is used, If an override is passed, other than 0, that value is set as the replicacount.
Love to hear your feedback.
Hi @appiepollo14. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
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 kubernetes-sigs/prow repository.
![openshift-ci[bot] avatar](https://avatars.githubusercontent.com/in/91280?v=4 "Published by a pub.dev verified publisher"){kind=small}
@swghosh can we continue? This fixes: https://issues.redhat.com/browse/RFE-6735
@swghosh who needs to resolve? Let’s merge soon In order to prevent conflict in the future.
/retitle RFE-6735: Allows for override replicas quantity to deploy a HA cert manager
/hold until CM-589 is concluded.
@appiepollo14: This pull request references RFE-6735 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 feature request to target the "4.20.0" version, but no target version was set.
In response to this:
As there was IMHO no way to deploy a HA cert manager, via the cert-manager-operator, I've added the option to override the replicas via the resource. If no override is done, the default (1) replicas is used, If an override is passed, other than 0, that value is set as the replicacount.
Love to hear your feedback.
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.
@swghosh the e2e failure is unrelated to this change. Can we please move forward after 8 months.
Walkthrough
Adds an optional OverrideReplicas field to the DeploymentConfig API and CRD schemas, implements logic to read and apply replica overrides in the deployment controller via a new hook, updates applyconfigurations and deep-copy code, and adds unit tests.
Changes
| Cohort / File(s) | Summary of Changes |
|---|---|
API type extensionapi/operator/v1alpha1/certmanager_types.go |
Added OverrideReplicas *int32 to DeploymentConfig with kubebuilder validation (minimum: 1) and JSON tag overrideReplicas,omitempty. |
Applyconfigurations updatepkg/operator/applyconfigurations/operator/v1alpha1/deploymentconfig.go |
Added OverrideReplicas *int32 to DeploymentConfigApplyConfiguration and fluent setter WithOverrideReplicas(value int32) *DeploymentConfigApplyConfiguration. |
Deep-copy generationapi/operator/v1alpha1/zz_generated.deepcopy.go |
Added nil-guarded deep-copy support for OverrideReplicas. |
CRD and bundle schemasconfig/crd/bases/operator.openshift.io_certmanagers.yaml, bundle/manifests/operator.openshift.io_certmanagers.yaml |
Inserted overrideReplicas (integer, format: int32, minimum: 1) with description "OverrideReplicas defines the number of replicas to run for an operand" under spec.[cainjectorConfig, controllerConfig, webhookConfig].properties. |
Deployment controller helpers and hookspkg/controller/deployment/deployment_helper.go, pkg/controller/deployment/deployment_overrides.go, pkg/controller/deployment/generic_deployment_controller.go |
Added getOverrideReplicasFor(certmanagerinformer.CertManagerInformer, string) (*int32, error); introduced overrideReplicasFunc type and withDeploymentReplicasOverrideHook(...) to apply non-nil overrides to deployment.Spec.Replicas; integrated the hook into the generic deployment controller. |
Testspkg/controller/deployment/deployment_helper_test.go |
Added TestGetOverrideReplicasFor to verify override extraction for controller, webhook, cainjector, and no-override cases (test appears duplicated). |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title Check | ✅ Passed | The title clearly summarizes the primary change by indicating that the pull request adds the ability to override replica counts for deploying a highly available cert manager, and it directly relates to the main functionality introduced without extraneous details. |
| Description Check | ✅ Passed | The description directly explains the new feature of overriding replica counts for HA deployments, aligns with the changeset, and provides context on default behavior and usage, making it sufficiently related to the pull request. |
| Docstring Coverage | ✅ Passed | No functions found in the changes. Docstring coverage check skipped. |
✨ Finishing touches
🧪 Generate unit tests
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
📜 Recent review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📥 Commits
Reviewing files that changed from the base of the PR and between f2482cc1739e1198a4217d2e1fd0a4194a82af33 and bc4ff39390b2b1ecb90e2c1fa110e9632fec64e7.
📒 Files selected for processing (1)
-
bundle/manifests/operator.openshift.io_certmanagers.yaml(3 hunks)
🔇 Additional comments (1)
bundle/manifests/operator.openshift.io_certmanagers.yaml (1)
180-185: Schema update mirrors DeploymentConfig addition.The new
overrideReplicasproperty (int32, minimum 1) is wired through for each operand, keeping the CRD bundle aligned with the API changes.Also applies to: 422-427, 709-714
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot] avatar](https://avatars.githubusercontent.com/in/347564?v=4 "Published by a pub.dev verified publisher"){kind=small}
@appiepollo14 Appreciate your contribution and patience! I believe we're very close to landing it.
Implemented the requested changes, thanks. Let's continu. It has just been 9 months now ;)
are the e2e failures related to this change?
@appiepollo14 No not relevant
In order to do some quick followup tests end-to-end, I've cherrypicked your commit into PR #315 and specially called out your amazing contribution. Apart from your code change I just added some more UTs and along with e2e tests. We're going to include this feature in the upcoming v1.18.0 release (targeting Oct.)
are the e2e failures related to this change?
@appiepollo14 No not relevant
In order to do some quick followup tests end-to-end, I've cherrypicked your commit into PR #315 and specially called out your amazing contribution. Apart from your code change I just added some more UTs and along with e2e tests. We're going to include this feature in the upcoming v1.18.0 release (targeting Oct.)
Ahhh so this pr will be discarded?
In order to do some quick followup tests end-to-end, I've cherrypicked your commit into PR #315 and specially called out your amazing contribution. Apart from your code change I just added some more UTs and along with e2e tests. We're going to include this feature in the upcoming v1.18.0 release (targeting Oct.)
@lunarwhite Thanks for the acknowledgement and for moving this along! I’d love for my original PRs to get merged directly where possible — happy to take on test additions or follow-ups if that helps. That said, I’m glad this feature made it into the upcoming release 🎉.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: appiepollo14, bharath-b-rh
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [bharath-b-rh]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@lunarwhite: This PR has been marked as verified by permerge.
In response to this:
/label qe-approved /verified by permerge
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.
@appiepollo14: This pull request references RFE-6735 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 feature request to target the "4.21.0" version, but no target version was set.
In response to this:
As there was IMHO no way to deploy a HA cert manager, via the cert-manager-operator, I've added the option to override the replicas via the resource. If no override is done, the default (1) replicas is used, If an override is passed, other than 0, that value is set as the replicacount.
Love to hear your feedback.
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.