Trigger deployments reconciliation when jaeger instance is created #1334
Conversation
Signed-off-by: Ruben Vargas Palma <[email protected]>
rubenvp8510
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1334 +/- ##
==========================================
- Coverage 87.41% 86.02% -1.39%
==========================================
Files 89 90 +1
Lines 5021 5125 +104
==========================================
+ Hits 4389 4409 +20
- Misses 467 548 +81
- Partials 165 168 +3
Continue to review full report at Codecov.
|
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
|
@jpkrohling @objectiser The changes looks really good. Regarding of codecov errors, it seems those are due uncovered lines when the client returns errors and one with assert error. I think those are hard to cover/test. Is this good enough to be merged as it is? |
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "dep-with-another-jaegers-label", | ||
| Namespace: "ns-without-annotation", | ||
| Labels: map[string]string{ |
There was a problem hiding this comment.
Should the inject annotation be included here, as in the deployment below - but as it is using a different Jaeger in the label it wouldn't be returned?
There was a problem hiding this comment.
Well, may be yes to be consistent with the expected states of a deployment, as I cannot imagine an scenario where we have the label and not the annotation.
On the other side, for what I think this is testing, it doesn't matter.
Whether this has or not an annotation, the first thing the syncOnJaegerChanges checks, is for the label, so it doesn't matter if we don't have an annotation.
There was a problem hiding this comment.
Agree, it was more to reflect a realistic deployment to avoid any failures later if the logic was to change.
There was a problem hiding this comment.
In that case. Yes , it should be added :)
There was a problem hiding this comment.
Should the inject annotation be included here, as in the deployment below - but as it is using a different Jaeger in the label it wouldn't be returned?
I left only the Label here as that's the only thing the code for this case will use. If we think we need to check both the annotation and the label, then both the test and the code need to be changed.
There was a problem hiding this comment.
I don't think we need to check both, Is more that the case should reflect a real deployment.
I understand that this test is not using the annotation, for this particular case it only check for the label. But if in the future we change this logic it could help to have the deployment as close as real as possible.
| } | ||
|
|
||
| deployments := appsv1.DeploymentList{} | ||
| err := r.client.List(context.Background(), &deployments) |
There was a problem hiding this comment.
Question
In the case of the operator watching one or a couple of namespaces (no cluster wide) Should we limit this to those namespaces?
There was a problem hiding this comment.
I don't think we have a way to restrict the namespaces being observed for injection? the WATCH_NAMESPACE part is which namespaces to check for CRs, not deployments.
@jpkrohling is that also your understanding?
There was a problem hiding this comment.
Ok, I thought WATCH_NAMESPACE restricts injection too. My bad.
There was a problem hiding this comment.
I'd expect the client.List to return the deployments for all namespaces where the jaeger-agent service account has access to. Perhaps QE could confirm that.
I think it's actually because there's a new test, which covers only one function, the new one. When there was 0% coverage, this package wasn't being counted at all.
Was it confirmed already that this is indeed the fix for the original problem? |
|
@jpkrohling It fixes one of the problems - and I believe the other PR (removing the persisted volume/mounts from the CR) addresses the other issue described, although we haven't been able to reproduce that. |
Signed-off-by: Ruben Vargas Palma <[email protected]>
Signed-off-by: Ruben Vargas Palma [email protected]
Fixes #1315