chore: sync upstream e2e tests to downstream#1179
Open
varshab1210 wants to merge 6 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 |
|
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:
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 nine new parallel E2E test files and extends three sequential test files validating operator runtime behaviors: deprecated logformat propagation, Dex token lifecycle, ApplicationSet resource cleanup, OpenShift serving-cert annotations, webhook secret sync, Prometheus metrics configuration, RoleBinding annotation isolation, ApplicationSet RBAC enforcement, and principal/agent ServiceMonitor toggles. Updates both ArgoCD CRD schema copies to define metrics, logging, resource constraints, webhook secret providers, sharding algorithms, and root-level spec fields. ChangesE2E Test Coverage Expansion and CRD Schema Enhancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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)
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 |
Contributor
|
LGTM |
Signed-off-by: Varsha B <vab@redhat.com>
Member
Author
|
/retest |
Member
Author
|
Member
Author
|
/retest |
2 similar comments
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/test v4.14-kuttl-parallel |
1 similar comment
Member
Author
|
/test v4.14-kuttl-parallel |
Signed-off-by: Varsha B <vab@redhat.com>
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
Member
Author
|
/test v4.14-kuttl-parallel |
Member
Author
|
/retest |
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: