USHIFT-6831: cert-manager Trust manager featuregate and test#6805
USHIFT-6831: cert-manager Trust manager featuregate and test#6805eslutsky wants to merge 2 commits into
Conversation
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[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
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}
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
| Oc Wait bundle ${TRUST_MANAGER_BUNDLE_NAME} | ||
| ... --for=jsonpath='{.status.conditions[0].reason}'=Synced --timeout=${DEFAULT_WAIT_TIMEOUT} |
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.
| 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.
eslutsky
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@eslutsky: This pull request references USHIFT-6831 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. |
|
/test all |
|
@eslutsky: The following tests failed, say
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. |
2 participants
Summary by CodeRabbit
New Features
Tests