Fix injecting volumes into rollover jobs #446
Conversation
Signed-off-by: Pavol Loffay <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #446 +/- ##
==========================================
+ Coverage 91.11% 91.57% +0.45%
==========================================
Files 64 64
Lines 3130 3132 +2
==========================================
+ Hits 2852 2868 +16
+ Misses 196 184 -12
+ Partials 82 80 -2
Continue to review full report at Codecov.
|
|
I think this deserves a test |
|
@pavolloffay Don't understand how this changes any behaviour, as appending an empty array should have no effect anyway? And not sure how this would fix the referenced problem? |
|
This is how pointers work in golang. The items have to be added to the array after they are modified. Or we could use an array of references instead. |
| @@ -125,6 +124,9 @@ func newProductionStrategy(jaeger *v1.Jaeger, existingSecrets []corev1.Secret) S | |||
| if indexCleaner != nil { | |||
| c.cronJobs = append(c.cronJobs, *indexCleaner) | |||
| } | |||
| if len(esRollover) > 0 { | |||
| c.cronJobs = append(c.cronJobs, esRollover...) | |||
There was a problem hiding this comment.
@objectiser to help you understand the code:
The esRollover arrays changes made on line 115 are not reflected if the esRollover array has been added to c.cronJobs before the modification.
There was a problem hiding this comment.
Yes, sorry - only looked at the changes, not the full code.
|
@pavolloffay agree needs a test, but otherwise looks good. |
Signed-off-by: Pavol Loffay <[email protected]>
pkg/strategy/production_test.go
Outdated
| for _, dep := range c.Deployments() { | ||
| if strings.HasSuffix(dep.Name, "-query") { | ||
| assert.Equal(t, 2, len(dep.Spec.Template.Spec.Containers)) | ||
| assert.Equal(t, "jaeger-agent", dep.Spec.Template.Spec.Containers[1].Name) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestInjectElasticsearch(t *testing.T) { | ||
| j := v1.NewJaeger("TestAgentSidecarIsInjectedIntoQueryForStreamingForProduction") |
There was a problem hiding this comment.
nit: Should use test name TestInjectElasticsearch
Signed-off-by: Pavol Loffay <[email protected]>
Resolves #434
Signed-off-by: Pavol Loffay [email protected]