fix(write-back): Duplicate image entries with different formats when updating two images#1037
Conversation
chengfang
requested review from
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1037 +/- ##
==========================================
+ Coverage 62.59% 62.89% +0.29%
==========================================
Files 15 15
Lines 2259 2269 +10
==========================================
+ Hits 1414 1427 +13
+ Misses 755 753 -2
+ Partials 90 89 -1 ☔ View full report in Codecov by Sentry. |
chengfang
force-pushed
the
duplicate.entries.in.write
branch
from
b6386e0 to
44f5996
Compare
| []v1alpha1.KustomizeImage{"nginx:latest@sha256:91734281c0ebfc6f1aea979cffeed5079cfe786228a71cc6f1f46a228cde6e34"}, | ||
| []v1alpha1.KustomizeImage{"nginx:latest@sha256:91734281c0ebfc6f1aea979cffeed5079cfe786228a71cc6f1f46a228cde6e34"}}, | ||
|
|
||
| {"2-images", []v1alpha1.KustomizeImage{"nginx:latest", |
There was a problem hiding this comment.
Cheng, do you have an example where the new image tag replaces the old image tag to test out the conditional statements in your new code in update.go?
There was a problem hiding this comment.
the sub-tests: with-tag-1, with-tag-sha, and 2-images all replace the existing tag with the new tag.
There was a problem hiding this comment.
I rechecked this again and yeah, the first test with-tag, the existing and the new tag are the same. The next three tests, by definition of your test case struct, the tags are different.
| newContainerImage.RegistryURL == existingContainerImage.RegistryURL { | ||
| found = true | ||
| if existingContainerImage.ImageTag == nil || | ||
| !(existingContainerImage.ImageTag).Equals(newContainerImage.ImageTag) { |
There was a problem hiding this comment.
From what I see, at least from the tests that I'm trying to run, the new image tag can be nil and so the newImage gets added
There was a problem hiding this comment.
In real applications, new image tag is unlikely to be nil. If it is, something already went wrong earlier.
But if the new tag is nil, it will cause nil pointer dereferece and panic. We could add a nil check to prevent that from happening:
if existingContainerImage.ImageTag == nil ||
(newContainerImage.ImageTag != nil && !(existingContainerImage.ImageTag).Equals(newContainerImage.ImageTag)) {
| for idx, existingImage := range *t.Kustomize.Images { | ||
| existingContainerImage := image.NewFromIdentifier(string(existingImage)) | ||
| if newContainerImage.ImageName == existingContainerImage.ImageName && | ||
| newContainerImage.RegistryURL == existingContainerImage.RegistryURL { |
There was a problem hiding this comment.
From the tests that I performed, RegistryURL is nil all the time. Is there another test that will test this condition with non-nil values?
There was a problem hiding this comment.
RegistryURL has a default value "" and should not be nil.
The first 4 sub-tests don't use any registry url, so they should be able to test this case.
There was a problem hiding this comment.
Ok, I see where the image RegistryURL is set. My comment can be resolved.
| require.Equal(t, lock3, state.repositoryLocks[repo2]) | ||
| } | ||
|
|
||
| func Test_mergeKustomizeOverride(t *testing.T) { |
There was a problem hiding this comment.
I was trying to test the new function from a higher level, at calling marshalParamsOverride instead. Do you think it is worthwhile to do it from that? eg. see the first couple of scenarios from the existing test Test_MarshalParamsOverride
There was a problem hiding this comment.
I can add a sub-test to Test_MarshalParamsOverride by setting the new images in app spec, to test out the changes from that level.
There was a problem hiding this comment.
Ok, I see the call to marshalParamsOverride
…updating two images Signed-off-by: Cheng Fang <cfang@redhat.com>
chengfang
force-pushed
the
duplicate.entries.in.write
branch
from
44f5996 to
d235ae4
Compare
4 participants
Fixes #1034
Modified
mergeKustomizeOverridemethod to have more control over setting and adding parameters to.argocd-source-*.yamltarget file.