Disable waiting on IDS job secret in managed clusters #1201
Conversation
vberezny
changed the title
| })).ToNot(HaveOccurred()) | ||
|
|
||
| result, err := r.Reconcile(ctx, reconcile.Request{}) | ||
| Expect(err).ShouldNot(HaveOccurred()) |
There was a problem hiding this comment.
Shouldn't we expect an error to be returned if it's waiting for a secret?
There was a problem hiding this comment.
I'll need to rethink these tests, realized the first one doesn't fail when it is forced to check for the IDS job secret.
There was a problem hiding this comment.
I updated the tests to make sure the change is properly covered. I am now deleting the IDS job secret and expecting the management cluster to call status.SetDegraded in response to not finding it in utils.ElasticSearch.
vberezny
force-pushed
the
vlad-managed-ids-job-secret
branch
from
c71c849
to
9f195a5
Compare
vberezny
force-pushed
the
vlad-managed-ids-job-secret
branch
from
9f195a5
to
ff57838
Compare
| Namespace: rmeta.OperatorNamespace(), | ||
| }, | ||
| } | ||
| err := c.Delete(ctx, idsSecret) |
There was a problem hiding this comment.
I think instead of doing this you should change the before each for the context above this context to not add the installer secret and put in a before each for the sibling contexts that need.
vberezny
force-pushed
the
vlad-managed-ids-job-secret
branch
2 times, most recently
from
c491f2e
to
fa47681
Compare
| @@ -74,6 +74,7 @@ var _ = Describe("IntrusionDetection controller tests", func() { | |||
| mockStatus.On("IsAvailable").Return(true) | |||
| mockStatus.On("OnCRFound").Return() | |||
| mockStatus.On("ClearDegraded") | |||
| mockStatus.On("SetDegraded", mock.Anything, mock.Anything).Return() | |||
There was a problem hiding this comment.
Doesn't this mean that tests the previously weren't setting a degraded status are now expected to, i.e. all the tests in the context "image reconciliation"? That seems like we shouldn't be changing that expectation...
There was a problem hiding this comment.
You're right, I'll throw them in a before each only for the tests that need SetDegraded
vberezny
force-pushed
the
vlad-managed-ids-job-secret
branch
from
fa47681
to
c192350
Compare
| _, err := r.Reconcile(ctx, reconcile.Request{}) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| // The missing secret should force utils.ElasticSearch to return a NotFound error which triggers r.status.SetDegraded. | ||
| Expect(mockStatus.AssertNumberOfCalls(nil, "SetDegraded", 1)).To(BeTrue()) |
There was a problem hiding this comment.
Instead of doing this it would probably be better to add an "AfterEach" in the top level context and use the AssertExpectations. That means that if you set "On" for a mock and that function wasn't called it would error out. I've been doing this for other projects, I think this was one of the first projects I started mocking with so I think I just forgot.
Description
[Fix] This PR disables waiting on the IDS job secret in managed clusters. This is done to reflect the changes that disable the creation of the secret in managed clusters.