Fix Remediation Events Occurrences and Names #125
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: razo7 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test 4.15-openshift-e2e |
razo7
force-pushed
the
fix-event-remediation-started
branch
from
be40de5
to
bfa13bf
Compare
|
/test 4.15-openshift-e2e |
| DeferCleanup(cleanupFar(), context.Background(), underTestFAR) | ||
| DeferCleanup(func() { | ||
| Expect(cleanupFar(context.Background(), underTestFAR)).To(Succeed()) | ||
| deleteErr := k8sClient.Get(ctx, client.ObjectKeyFromObject(underTestFAR), &v1alpha1.FenceAgentsRemediation{}) |
There was a problem hiding this comment.
I do not think that this is much different that putting the verifyEvent directly inside the cleanup function.
Is this necessary for a reason? Could we instead explicitly call the appropriate verifyEvent inside the test body?
There was a problem hiding this comment.
I am confused. From your last comment I understood that you prefer that I won't "hide" a check (verifyEvent(...utils.EventMessageRemoveRemediationCR)) in a function, so I took it out of from the function.
I do not think that this is much different that putting the verifyEvent directly inside the cleanup function.
I think that this way is clearer for showing that we test this verifyEvent after cleaning the CR.
Is this necessary for a reason? Could we instead explicitly call the appropriate verifyEvent inside the test body?
Normally we don't verify that the CR was deleted using this event, but it looks like a good sign. We can create a test that verifies that if a CR was created and when it is deleted then the finalizer was deleted as well. Would it make sense to follow this logic or leave it as a "step" in DeferCleanup or even as one line in cleanupFar as I introduced in the beginning?
There was a problem hiding this comment.
I am confused.
I am sorry, I should have been more clear. I was concerned by the call to verifyEvent not being in the Test body as other verifyEvent calls are. So having it in the cleanup function or called after the cleanup function, but still not in the Test body looks the same to me.
We can create a test that verifies that if a CR was created
that's too much logic in a test (that should be simple to read). My idea was that inside each Test body you should know if the CR is created or not, so you can explicitly call verifyEvent or not, without any specific check. However, I was not sure whether this is easy to do or not, that's why I asked if there was a specific reason not to do so.
There was a problem hiding this comment.
OK, now I understand your motivation about the location of the questionable verifyEvent
My idea was that inside each Test body you should know if the CR is created or not, so you can explicitly call verifyEvent or not, without any specific check.
I am trying to test the emitting of the event remove finalizer which signifies that the CR was deleted. Until now we have identified that by searching and not finding the CR, and using verifyEvent is just another way to verify that. It is a double-check, in addition to the old one. This event should be emitted whenever the CR is deleted, thus IMO it makes sense to verify that after the old check and it can be done after each deletion of the CR.
There was a problem hiding this comment.
we can deal with this in a next PR, not urgent to fix it here
Previously, it caused some confusion as remediation started multiple times, while it could resembles reconciliation and not remediation
Seek for FAR CR deletion event only when there isn't an API 'is not found' error'
razo7
force-pushed
the
fix-event-remediation-started
branch
from
bfa13bf
to
3540b62
Compare
|
/test 4.15-openshift-e2e |
|
as per my comment here /lgtm |
|
/retest |
Fix https://issues.redhat.com/browse/ECOPROJECT-1819