chore(e2e): sync DefaultClusterScopedRoleDisabled tests for principal/agent#1211
chore(e2e): sync DefaultClusterScopedRoleDisabled tests for principal/agent#1211cjcocokrisp wants to merge 6 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 @cjcocokrisp. Thanks for your PR. I'm waiting for a redhat-developer 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe E2E tests update ArgoCD agent image expectations to v0.5.1, refine resource verification, update module versions, and add coverage for toggling cluster-scoped RBAC through ChangesArgoCD agent E2E validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant ArgoCDCR
participant Operator
participant KubernetesAPI
E2ETest->>ArgoCDCR: Toggle DefaultClusterScopedRoleDisabled
ArgoCDCR->>Operator: Reconcile ArgoCD configuration
Operator->>KubernetesAPI: Create or delete ClusterRole and ClusterRoleBinding
E2ETest->>KubernetesAPI: Verify RBAC resource state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
`@test/openshift/e2e/ginkgo/sequential/1-052_validate_argocd_agent_agent_test.go`:
- Line 627: Correct the `By()` description in the relevant test to say “agent
enabled” instead of “principal enabled,” keeping the rest of the message
unchanged.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 58eada41-5881-4fd5-aecb-d9153006f809
📒 Files selected for processing (2)
test/openshift/e2e/ginkgo/sequential/1-051_validate_argocd_agent_principal_test.gotest/openshift/e2e/ginkgo/sequential/1-052_validate_argocd_agent_agent_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
|
/ok-to-test |
|
/retest |
1 similar comment
|
/retest |
|
/retest-required |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
go.mod (1)
8-8: 🗄️ Data Integrity & Integration | 🔵 TrivialRun the required E2E suites against this exact operator commit.
The updated operator module directly supplies the Agent API and RBAC reconciliation behavior exercised by the principal and agent tests. Confirm successful E2E results, including the
DefaultClusterScopedRoleDisabledtransitions, before merging.🤖 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 `@go.mod` at line 8, Update the github.com/argoproj-labs/argocd-operator dependency to the exact commit required by the E2E suites, then run the required principal and agent E2E tests and verify successful DefaultClusterScopedRoleDisabled transitions before merging.Source: Linked repositories
🤖 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 `@go.mod`:
- Line 8: Update the github.com/argoproj-labs/argocd-operator dependency to the
exact commit required by the E2E suites, then run the required principal and
agent E2E tests and verify successful DefaultClusterScopedRoleDisabled
transitions before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3607b822-01fd-42af-af93-90496bcbfdde
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/openshift/e2e/ginkgo/sequential/1-052_validate_argocd_agent_agent_test.go (1)
648-659: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert that namespaced RBAC survives both toggles.
The test verifies
RoleandRoleBindingonly while the flag is initiallytrue. A regression that deletes namespaced RBAC during either transition would still pass. Add existence or consistency checks after re-enabling and after disabling cluster-scoped RBAC again.Proposed assertion additions
Eventually(clusterRoleBinding, "60s", "2s").Should(k8sFixture.ExistByName()) + Eventually(role, "60s", "2s").Should(k8sFixture.ExistByName()) + Eventually(roleBinding, "60s", "2s").Should(k8sFixture.ExistByName()) By("Setting DefaultClusterScopedRoleDisabled again deletes clusterRole and clusterRoleBinding") ... Consistently(clusterRoleBinding, "10s", "2s").Should(k8sFixture.NotExistByName()) + Consistently(role, "10s", "2s").Should(k8sFixture.ExistByName()) + Consistently(roleBinding, "10s", "2s").Should(k8sFixture.ExistByName())🤖 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/openshift/e2e/ginkgo/sequential/1-052_validate_argocd_agent_agent_test.go` around lines 648 - 659, Extend the test around the DefaultClusterScopedRoleDisabled toggles to assert that the namespaced Role and RoleBinding remain present after re-enabling and after disabling cluster-scoped RBAC again. Use the existing namespaced RBAC fixture references and add appropriate Eventually or Consistently existence checks alongside the clusterRole and clusterRoleBinding assertions.Source: Linked repositories
🤖 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.
Outside diff comments:
In
`@test/openshift/e2e/ginkgo/sequential/1-052_validate_argocd_agent_agent_test.go`:
- Around line 648-659: Extend the test around the
DefaultClusterScopedRoleDisabled toggles to assert that the namespaced Role and
RoleBinding remain present after re-enabling and after disabling cluster-scoped
RBAC again. Use the existing namespaced RBAC fixture references and add
appropriate Eventually or Consistently existence checks alongside the
clusterRole and clusterRoleBinding assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 18c4773b-71ea-4366-8ce8-6668de46ba99
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modtest/openshift/e2e/ginkgo/sequential/1-051_validate_argocd_agent_principal_test.gotest/openshift/e2e/ginkgo/sequential/1-052_validate_argocd_agent_agent_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/openshift/e2e/ginkgo/sequential/1-051_validate_argocd_agent_principal_test.go
…/agent Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
Signed-off-by: Christopher Coco <ccoco@redhat.com>
…verwritten by copy Signed-off-by: Christopher Coco <ccoco@redhat.com>
55bbb43 to
45f7fbc
Compare
|
@cjcocokrisp: 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
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
In Argo CD Operator PR #2237 there is a new e2e test added for both verifying the principal and agent for Argo CD Agent. This PR syncs those tests to this repo.
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
NA
Test acceptance criteria:
How to test changes / Special notes to the reviewer:
Run e2e tests and ensure that they pass.