added E2E for role ownership validation#1213
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
[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 |
36c1c33 to
042432b
Compare
Signed-off-by: Alka Kumari <alkumari@redhat.com>
042432b to
1ce87c7
Compare
ranakan19
left a comment
ranakan19
left a comment
There was a problem hiding this comment.
left some comments
I'm a bit unsure how the tests are checking for role ownership. I see the tests are verifying UID of the roles and rolebinding, but don't see anything about ownership. Could you also add references to the bug in the description for easier review?
| ctx = context.Background() | ||
| }) | ||
|
|
||
| It("validates that the role bug is fixed", func() { |
There was a problem hiding this comment.
nit: we won't remember which role bug we are referring to here, we should rename it for our future selves.
| afterServerRoleBindingReconcileUid := newServerClusterRoleBinding.GetUID() | ||
|
|
||
| By("comparing the UID to check if the ClusterRole was recreated") | ||
| Expect(initialControllerUid).To(Equal(afterControllerReconcileUid), "the ClusterRole was recreated") |
There was a problem hiding this comment.
if the intial and after reconcile uid are the same, doesn't that imply that it was not recreated?
| Eventually(newServerClusterRoleBinding).Should(k8sFixture.ExistByName()) | ||
|
|
||
| By("fetching UID of the clusterrole after reconciliation") | ||
| afterControllerReconcileUid := newControllerClusterRole.GetUID() |
There was a problem hiding this comment.
could you rename other variables in L169-L175 as well?
| afterControllerReconcileUid := newControllerClusterRole.GetUID() | |
| afterReconcileControllerUid := newControllerClusterRole.GetUID() |
| var _ = Describe("GitOps Operator Sequential E2E Tests", func() { | ||
|
|
||
| const ( | ||
| clusterRoleName = "argocd-agent-argocd-agent-agent-agent" |
There was a problem hiding this comment.
curious if this is a test name for the role or the actual role name? lots of repeating agent in the name.
if this is just for test, i don't see a difference in the name or role and rolebinding name, could we try to have say a suffix in the name for role and rb?
| }, | ||
| } | ||
| Eventually(service).Should(k8sFixture.ExistByName()) | ||
| Expect(string(service.Spec.Type)).To(Equal("ClusterIP")) |
There was a problem hiding this comment.
test name gives an indication service type is checked dynamically to either be clusterIP or LoadBalancer, but this line is hardcoded to check only for ClusterIP.
| Expect(string(service.Spec.Type)).To(Equal("ClusterIP")) | ||
|
|
||
| } | ||
| By("verifying primary agent Deployment has expected values") |
There was a problem hiding this comment.
| By("verifying primary agent Deployment has expected values") | |
| By("verifying primary agent Deployment has expected labels") |
2 participants
What type of PR is this?
What does this PR do / why we need it:
added E2E for role ownership validation
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: