Fix issue #1668 about auto renew certificat on mutating / validatinf#1669
Fix issue #1668 about auto renew certificat on mutating / validatinf#1669disaster37 wants to merge 1 commit into
Conversation
… validation deployment
disaster37
requested review from
akurinnoy,
btjd,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: disaster37 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 |
|
Hi @disaster37. Thanks for your PR. I'm waiting for a devfile 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. |
📝 WalkthroughWalkthroughAdds ChangesWebhook cert-manager annotations
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webhook/workspace/mutating_cfg.go (1)
160-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the annotation to a shared constant.
The
cert-manager.io/inject-ca-fromkey anddevworkspace-controller-serving-certname are duplicated verbatim invalidating_cfg.go(Lines 41-43). Centralizing this in a shared constant/helper (e.g., alongsideserver.WebhookServerAppLabels()) would prevent the two copies from drifting if the secret/certificate name ever changes.♻️ Proposed refactor
+// in a shared location, e.g. webhook/workspace/server package +const CertManagerServingCertName = "devworkspace-controller-serving-cert" + +func CertManagerInjectCAAnnotation(namespace string) map[string]string { + return map[string]string{ + "cert-manager.io/inject-ca-from": fmt.Sprintf("%s/%s", namespace, CertManagerServingCertName), + } +}- Annotations: map[string]string{ - "cert-manager.io/inject-ca-from": fmt.Sprintf("%s/devworkspace-controller-serving-cert", namespace), - }, + Annotations: server.CertManagerInjectCAAnnotation(namespace),🤖 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 `@webhook/workspace/mutating_cfg.go` around lines 160 - 162, The webhook CA injection annotation and serving cert name are duplicated in mutating_cfg.go and validating_cfg.go, so extract them into a shared constant or helper near the webhook/server label helpers (for example alongside server.WebhookServerAppLabels) and use that in both places. Update the annotation map construction in the webhook config methods to reference the shared symbol instead of hardcoding cert-manager.io/inject-ca-from and devworkspace-controller-serving-cert, so both configs stay in sync if the name changes.
🤖 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.
Nitpick comments:
In `@webhook/workspace/mutating_cfg.go`:
- Around line 160-162: The webhook CA injection annotation and serving cert name
are duplicated in mutating_cfg.go and validating_cfg.go, so extract them into a
shared constant or helper near the webhook/server label helpers (for example
alongside server.WebhookServerAppLabels) and use that in both places. Update the
annotation map construction in the webhook config methods to reference the
shared symbol instead of hardcoding cert-manager.io/inject-ca-from and
devworkspace-controller-serving-cert, so both configs stay in sync if the name
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 300da1dc-77c7-49ef-b60d-15c9b809d86d
📒 Files selected for processing (2)
webhook/workspace/mutating_cfg.gowebhook/workspace/validating_cfg.go
1 participant
What does this PR do?
Add cert-manager annotations on mutating and validating depoloyment to auto renew certificate and auto restart pod when it's done.
What issues does this PR fix or reference?
Fix issue #1668
Is it tested? How?
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit