chore: sync upstream e2e tests to downstream#1179
Open
varshab1210 wants to merge 3 commits into
Open
Conversation
Signed-off-by: Varsha B <vab@redhat.com>
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds and extends eleven Ginkgo E2E test files across parallel and sequential suites, and expands the ArgoCD CRD schema with new configuration surfaces. E2E tests cover: deprecated lowercase ChangesE2E Test Coverage Expansion and CRD Schema Enhancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
varshab1210
commented
Jun 16, 2026
| defer cleanupFunc() | ||
|
|
||
| argoCD := &argov1beta1api.ArgoCD{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "example-argocd-clear-gh", Namespace: ns.Name}, |
Member
Author
There was a problem hiding this comment.
@aali309 I had to change the name as I saw the error
Route.route.openshift.io "example-argocd-clear-github-server" is invalid: spec.host: Invalid value: "example-argocd-clear-github-server-gitops-e2e-test-0b13d83e-eda2.apps.psi-421-002.ocp-gitops-qe.com": must be no more than 63 characters
Looks like we need something similar to https://redhat.atlassian.net/issues?jql=reporter%20IN%20(62332b7550cceb00707ad539)%20AND%20type%20%3D%20Bug%20AND%20summary%20~%20%22host%22&selectedIssue=GITOPS-7315
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/parallel/1-048_validate_controller_sharding_test.go`:
- Around line 119-122: The assertion at line 121 negates the
HaveContainerWithEnvVar matcher with a specific key and value, which only passes
if the environment variable exists with a different value. This is too weak and
can miss regressions. Instead of using ShouldNot with HaveContainerWithEnvVar
matching a specific key-value pair, use a matcher that asserts the
ARGOCD_CONTROLLER_SHARDING_ALGORITHM environment variable key is completely
absent from the app controller StatefulSet, regardless of its value. Replace the
negated key-value matcher with an assertion that explicitly checks for the
absence of the environment variable key itself.
In
`@test/openshift/e2e/ginkgo/sequential/1-037_validate_applicationset_in_any_namespace_test.go`:
- Around line 643-656: The nested loop structure selecting the
operatorClusterRoleName picks the first matching ClusterRoleBinding for the
ServiceAccount without verifying it grants the required permissions, which can
lead to selecting the wrong RBAC object. Instead of selecting based only on
matching the ServiceAccount name and namespace, implement a deterministic
selection criterion by resolving the candidate ClusterRoles and validating that
the selected ClusterRole grants the necessary permissions (such as `list` on
`namespaces`) before using it for mutation. This ensures the correct operator
ClusterRole is chosen for the test rather than any arbitrary matching binding.
- Around line 663-691: The rule rewriting logic in the loop iterating through
operatorClusterRole.Rules does not handle PolicyRules containing multiple
resources correctly. When a rule contains both "namespaces" and other resources,
the current code removes "list" from the verbs but applies this modified verb
set to all resources in the rule, not just namespaces. To fix this, when you
find "namespaces" in the resources list, check if there are other resources
present as well. If it is a mixed rule with multiple resources, create two
separate PolicyRule objects: one with only "namespaces" as the resource and the
modified verbs (without "list"), and another with the non-namespaces resources
and the original verbs. If "namespaces" is the only resource, proceed with the
existing logic of removing "list" and adding the modified rule to modifiedRules.
🪄 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: ad187eb0-4a14-46b0-ae80-5a89cd221a7f
📒 Files selected for processing (11)
test/openshift/e2e/ginkgo/parallel/1-043_validate_log_level_format_test.gotest/openshift/e2e/ginkgo/parallel/1-048_validate_controller_sharding_test.gotest/openshift/e2e/ginkgo/parallel/1-095_validate_dex_clientsecret_test.gotest/openshift/e2e/ginkgo/parallel/1-125_validate_applicationset_cleanup_when_spec_field_omitted_test.gotest/openshift/e2e/ginkgo/parallel/1-125_validate_server_serving_cert_annotation_restore_test.gotest/openshift/e2e/ginkgo/parallel/1-126_validate_declarative_webhook_secrets_test.gotest/openshift/e2e/ginkgo/parallel/1-126_validate_servicemonitor_metrics_config_test.gotest/openshift/e2e/ginkgo/parallel/1-131_validate_rolebinding_no_tracking_annotation_propagation_test.gotest/openshift/e2e/ginkgo/sequential/1-037_validate_applicationset_in_any_namespace_test.gotest/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)
Comment on lines
+643
to
+656
| for i := range crbList.Items { | ||
| for _, subj := range crbList.Items[i].Subjects { | ||
| if subj.Kind == "ServiceAccount" && subj.Namespace == operatorDeployment.Namespace && subj.Name == saName { | ||
| operatorClusterRoleName = crbList.Items[i].RoleRef.Name | ||
| break | ||
| } | ||
| } | ||
| if operatorClusterRoleName != "" { | ||
| break | ||
| } | ||
| } | ||
| if operatorClusterRoleName == "" { | ||
| Skip("Operator ClusterRoleBinding not found") | ||
| } |
There was a problem hiding this comment.
Select the operator ClusterRoleBinding deterministically before mutation.
This loop picks the first ClusterRoleBinding matching the ServiceAccount, which can point to a non-operator ClusterRole. That can make the test skip incorrectly or mutate the wrong RBAC object.
Use a deterministic selection criterion (e.g., resolve candidate ClusterRoles and choose one that actually grants list on namespaces) before editing rules.
🤖 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-037_validate_applicationset_in_any_namespace_test.go`
around lines 643 - 656, The nested loop structure selecting the
operatorClusterRoleName picks the first matching ClusterRoleBinding for the
ServiceAccount without verifying it grants the required permissions, which can
lead to selecting the wrong RBAC object. Instead of selecting based only on
matching the ServiceAccount name and namespace, implement a deterministic
selection criterion by resolving the candidate ClusterRoles and validating that
the selected ClusterRole grants the necessary permissions (such as `list` on
`namespaces`) before using it for mutation. This ensures the correct operator
ClusterRole is chosen for the test rather than any arbitrary matching binding.
Comment on lines
+663
to
+691
| for _, r := range operatorClusterRole.Rules { | ||
| replaced := false | ||
| for _, res := range r.Resources { | ||
| if res == "namespaces" { | ||
| replaced = true | ||
| for _, v := range r.Verbs { | ||
| if v == "list" { | ||
| removedListFromNamespaces = true | ||
| break | ||
| } | ||
| } | ||
| var newVerbs []string | ||
| for _, v := range r.Verbs { | ||
| if v != "list" { | ||
| newVerbs = append(newVerbs, v) | ||
| } | ||
| } | ||
| if len(newVerbs) > 0 { | ||
| modifiedRules = append(modifiedRules, rbacv1.PolicyRule{ | ||
| APIGroups: r.APIGroups, Resources: r.Resources, Verbs: newVerbs, | ||
| ResourceNames: r.ResourceNames, NonResourceURLs: r.NonResourceURLs, | ||
| }) | ||
| } | ||
| break | ||
| } | ||
| } | ||
| if !replaced { | ||
| modifiedRules = append(modifiedRules, r) | ||
| } |
There was a problem hiding this comment.
The rule rewrite revokes list from more than namespaces in mixed rules.
When a PolicyRule contains namespaces plus other resources, the rewritten rule keeps all resources but drops list, unintentionally revoking list for unrelated resources too. That broadens the failure mode and weakens the test’s causal signal.
Split mixed rules: keep original verbs for non-namespaces resources, and apply the verb removal only to a namespaces-only rule.
🤖 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-037_validate_applicationset_in_any_namespace_test.go`
around lines 663 - 691, The rule rewriting logic in the loop iterating through
operatorClusterRole.Rules does not handle PolicyRules containing multiple
resources correctly. When a rule contains both "namespaces" and other resources,
the current code removes "list" from the verbs but applies this modified verb
set to all resources in the rule, not just namespaces. To fix this, when you
find "namespaces" in the resources list, check if there are other resources
present as well. If it is a mixed rule with multiple resources, create two
separate PolicyRule objects: one with only "namespaces" as the resource and the
modified verbs (without "list"), and another with the non-namespaces resources
and the original verbs. If "namespaces" is the only resource, proceed with the
existing logic of removing "list" and adding the modified rule to modifiedRules.
CRD changes Assisted-by: cursor
cjcocokrisp
reviewed
Jun 16, 2026
|
|
||
| By("checking if ARGOCD_CONTROLLER_SHARDING_ALGORITHM env var is not set in app controller StatefulSet") | ||
| Eventually(statefulSet).Should(k8sFixture.ExistByName()) | ||
| Eventually(statefulSet, "60s", "5s").ShouldNot(statefulset.HaveContainerWithEnvVar("ARGOCD_CONTROLLER_SHARDING_ALGORITHM", "round-robin", 0), "Statefulset should not have ARGOCD_CONTROLLER_SHARDING_ALGORITHM") |
There was a problem hiding this comment.
I agree with coderabbit's comment. The suggested fix also seems good to me as well and I have commented below the version that I used to test it.
Eventually(func() bool {
if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(statefulSet), statefulSet); err != nil {
return false
}
if len(statefulSet.Spec.Template.Spec.Containers) == 0 {
return false
}
for _, env := range statefulSet.Spec.Template.Spec.Containers[0].Env {
if env.Name == "ARGOCD_CONTROLLER_SHARDING_ALGORITHM" {
return false
}
}
return true
}, "60s", "5").Should(BeTrue(), "StatefulSet should have no env variable named ARGOCD_CONTROLLER_SHARDING_ALGORITHM")Signed-off-by: Varsha B <vab@redhat.com>
Member
Author
|
/test v4.14-ci-index-gitops-operator-bundle |
Member
Author
|
/test v4.14-kuttl-parallel |
|
@varshab1210: The following test 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. |
Contributor
|
LGTM |
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
3 participants
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.
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
sync upstream argocd-operator e2e tests to downstream
Tested locally against 1.21 RC
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
Test acceptance criteria:
How to test changes / Special notes to the reviewer: