cert-manager Trust manager featuregate and test#6805
Draft
eslutsky wants to merge 2 commits into
Draft
Conversation
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eslutsky 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
Contributor
WalkthroughThe PR adds Trust Manager support to cert-manager by enabling the feature flag in the operator via Kustomize patch, defining a new TrustManager custom resource manifest, and introducing comprehensive Robot test coverage with lifecycle management keywords and three functional test cases for CA secret and Bundle synchronization scenarios. ChangesTrust Manager Support for cert-manager
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/suites/optional/cert-manager.robot (1)
54-54: 💤 Low valueUnused variable.
${TRUST_MANAGER_DEPLOYMENT}is defined but never referenced in this file.🧹 Remove or use the variable
-${TRUST_MANAGER_DEPLOYMENT} cert-manager-operator-controller-manager🤖 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/suites/optional/cert-manager.robot` at line 54, ${TRUST_MANAGER_DEPLOYMENT} is defined but unused; either remove the variable declaration or use it where the cert-manager operator deployment name is needed (e.g., replace hard-coded "cert-manager-operator-controller-manager" occurrences with ${TRUST_MANAGER_DEPLOYMENT} or reference it in the test keyword that interacts with the trust manager). Update the robot file by deleting the unused variable line if not needed, or wire ${TRUST_MANAGER_DEPLOYMENT} into the relevant test steps/keywords to consume the value.
🤖 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 `@test/suites/optional/cert-manager.robot`:
- Around line 150-151: The jsonpath used in the Oc Wait call is fragile because
it assumes the condition is at index 0; update the jsonpath in the Oc Wait that
references TRUST_MANAGER_BUNDLE_NAME to check all conditions instead of
.status.conditions[0].reason (e.g. use .status.conditions[*].reason or a filter
by condition type) so the wait asserts any condition reason equals "Synced"
rather than relying on index 0; modify the Oc Wait invocation that currently
uses .status.conditions[0].reason to use the wildcard/filter form to match line
122-123's fix approach.
---
Nitpick comments:
In `@test/suites/optional/cert-manager.robot`:
- Line 54: ${TRUST_MANAGER_DEPLOYMENT} is defined but unused; either remove the
variable declaration or use it where the cert-manager operator deployment name
is needed (e.g., replace hard-coded "cert-manager-operator-controller-manager"
occurrences with ${TRUST_MANAGER_DEPLOYMENT} or reference it in the test keyword
that interacts with the trust manager). Update the robot file by deleting the
unused variable line if not needed, or wire ${TRUST_MANAGER_DEPLOYMENT} into the
relevant test steps/keywords to consume the value.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7d657932-8817-491b-8597-8605b0f20abd
📒 Files selected for processing (3)
assets/optional/cert-manager/manager/kustomization.yamltest/assets/cert-manager/trust-manager-cr.yamltest/suites/optional/cert-manager.robot
Comment on lines
+150
to
+151
| Oc Wait bundle ${TRUST_MANAGER_BUNDLE_NAME} | ||
| ... --for=jsonpath='{.status.conditions[0].reason}'=Synced --timeout=${DEFAULT_WAIT_TIMEOUT} |
Contributor
There was a problem hiding this comment.
Same fragile index issue.
Apply the same fix as line 122-123.
🔧 Proposed fix
Oc Wait bundle ${TRUST_MANAGER_BUNDLE_NAME}
- ... --for=jsonpath='{.status.conditions[0].reason}'=Synced --timeout=${DEFAULT_WAIT_TIMEOUT}
+ ... --for=jsonpath='{.status.conditions[?(@.type=="Synced")].status}'=True --timeout=${DEFAULT_WAIT_TIMEOUT}📝 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
| Oc Wait bundle ${TRUST_MANAGER_BUNDLE_NAME} | |
| ... --for=jsonpath='{.status.conditions[0].reason}'=Synced --timeout=${DEFAULT_WAIT_TIMEOUT} | |
| Oc Wait bundle ${TRUST_MANAGER_BUNDLE_NAME} | |
| ... --for=jsonpath='{.status.conditions[?(@.type=="Synced")].status}'=True --timeout=${DEFAULT_WAIT_TIMEOUT} |
🤖 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/suites/optional/cert-manager.robot` around lines 150 - 151, The jsonpath
used in the Oc Wait call is fragile because it assumes the condition is at index
0; update the jsonpath in the Oc Wait that references TRUST_MANAGER_BUNDLE_NAME
to check all conditions instead of .status.conditions[0].reason (e.g. use
.status.conditions[*].reason or a filter by condition type) so the wait asserts
any condition reason equals "Synced" rather than relying on index 0; modify the
Oc Wait invocation that currently uses .status.conditions[0].reason to use the
wildcard/filter form to match line 122-123's fix approach.
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 by CodeRabbit
New Features
Tests