Skip to content

fix(write-back): Duplicate image entries with different formats when updating two images #1037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chengfang merged 1 commit into from
Feb 25, 2025
Merged

fix(write-back): Duplicate image entries with different formats when updating two images #1037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions pkg/argocd/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,13 +551,24 @@ func mergeHelmOverride(t *helmOverride, o *helmOverride) {
}

func mergeKustomizeOverride(t *kustomizeOverride, o *kustomizeOverride) {
for _, image := range *o.Kustomize.Images {
idx := t.Kustomize.Images.Find(image)
if idx != -1 {
(*t.Kustomize.Images)[idx] = image
continue
for _, newImage := range *o.Kustomize.Images {
found := false
newContainerImage := image.NewFromIdentifier(string(newImage))
for idx, existingImage := range *t.Kustomize.Images {
existingContainerImage := image.NewFromIdentifier(string(existingImage))
if newContainerImage.ImageName == existingContainerImage.ImageName &&
newContainerImage.RegistryURL == existingContainerImage.RegistryURL {
Copy link
Copy Markdown

@keithchong keithchong Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

@chengfang chengfang Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@keithchong keithchong Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see where the image RegistryURL is set. My comment can be resolved.

found = true
if existingContainerImage.ImageTag == nil ||
(newContainerImage.ImageTag != nil && !(existingContainerImage.ImageTag).Equals(newContainerImage.ImageTag)) {
(*t.Kustomize.Images)[idx] = newImage
}
break
}
}
if !found {
*t.Kustomize.Images = append(*t.Kustomize.Images, newImage)
}
*t.Kustomize.Images = append(*t.Kustomize.Images, image)
}
}

Expand Down
101 changes: 101 additions & 0 deletions pkg/argocd/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1219,6 +1219,49 @@ kustomize:
assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(string(yaml)))
})

t.Run("Merge images param", func(t *testing.T) {
expected := `
kustomize:
images:
- existing:latest
- updated:latest
- new
`
app := v1alpha1.Application{
ObjectMeta: v1.ObjectMeta{
Name: "testapp",
Annotations: map[string]string{
"argocd-image-updater.argoproj.io/image-list": "nginx",
},
},
Spec: v1alpha1.ApplicationSpec{
Source: &v1alpha1.ApplicationSource{
RepoURL: "https://example.com/example",
TargetRevision: "main",
Kustomize: &v1alpha1.ApplicationSourceKustomize{
Images: v1alpha1.KustomizeImages{
"new",
"updated:latest",
},
},
},
},
Status: v1alpha1.ApplicationStatus{
SourceType: v1alpha1.ApplicationSourceTypeKustomize,
},
}
originalData := []byte(`
kustomize:
images:
- existing:latest
- updated:old
`)
yaml, err := marshalParamsOverride(&app, originalData)
require.NoError(t, err)
assert.NotEmpty(t, yaml)
assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(string(yaml)))
})

t.Run("Empty Kustomize source", func(t *testing.T) {
app := v1alpha1.Application{
ObjectMeta: v1.ObjectMeta{
Expand Down Expand Up @@ -3643,3 +3686,61 @@ func Test_GetRepositoryLock(t *testing.T) {
require.NotNil(t, state.repositoryLocks[repo2])
require.Equal(t, lock3, state.repositoryLocks[repo2])
}

func Test_mergeKustomizeOverride(t *testing.T) {
Copy link
Copy Markdown

@keithchong keithchong Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

@chengfang chengfang Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@keithchong keithchong Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see the call to marshalParamsOverride

tests := []struct {
name string
existing v1alpha1.KustomizeImages
new v1alpha1.KustomizeImages
expected v1alpha1.KustomizeImages
}{
{"with-tag", []v1alpha1.KustomizeImage{"nginx:foo"},
[]v1alpha1.KustomizeImage{"nginx:foo"},
[]v1alpha1.KustomizeImage{"nginx:foo"}},
{"no-tag", []v1alpha1.KustomizeImage{"nginx:foo"},
[]v1alpha1.KustomizeImage{"nginx"},
[]v1alpha1.KustomizeImage{"nginx:foo"}},
{"with-tag-1", []v1alpha1.KustomizeImage{"nginx"},
[]v1alpha1.KustomizeImage{"nginx:latest"},
[]v1alpha1.KustomizeImage{"nginx:latest"}},
{"with-tag-sha", []v1alpha1.KustomizeImage{"nginx:latest"},
[]v1alpha1.KustomizeImage{"nginx:latest@sha256:91734281c0ebfc6f1aea979cffeed5079cfe786228a71cc6f1f46a228cde6e34"},
[]v1alpha1.KustomizeImage{"nginx:latest@sha256:91734281c0ebfc6f1aea979cffeed5079cfe786228a71cc6f1f46a228cde6e34"}},

{"2-images", []v1alpha1.KustomizeImage{"nginx:latest",
Copy link
Copy Markdown

@keithchong keithchong Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

@chengfang chengfang Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the sub-tests: with-tag-1, with-tag-sha, and 2-images all replace the existing tag with the new tag.

Copy link
Copy Markdown

@keithchong keithchong Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"bitnami/nginx:latest@sha256:1a2fe3f9f6d1d38d5a7ee35af732fdb7d15266ec3dbc79bbc0355742cd24d3ec"},
[]v1alpha1.KustomizeImage{"nginx:latest@sha256:91734281c0ebfc6f1aea979cffeed5079cfe786228a71cc6f1f46a228cde6e34",
"bitnami/nginx@sha256:1a2fe3f9f6d1d38d5a7ee35af732fdb7d15266ec3dbc79bbc0355742cd24d3ec"},
[]v1alpha1.KustomizeImage{"nginx:latest@sha256:91734281c0ebfc6f1aea979cffeed5079cfe786228a71cc6f1f46a228cde6e34",
"bitnami/nginx:latest@sha256:1a2fe3f9f6d1d38d5a7ee35af732fdb7d15266ec3dbc79bbc0355742cd24d3ec"}},

{"with-registry", []v1alpha1.KustomizeImage{"quay.io/nginx:latest"},
[]v1alpha1.KustomizeImage{"quay.io/nginx:latest"},
[]v1alpha1.KustomizeImage{"quay.io/nginx:latest"}},
{"with-registry-1", []v1alpha1.KustomizeImage{"quay.io/nginx:latest"},
[]v1alpha1.KustomizeImage{"docker.io/nginx:latest"},
[]v1alpha1.KustomizeImage{"docker.io/nginx:latest", "quay.io/nginx:latest"}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
existingImages := kustomizeOverride{
Kustomize: kustomizeImages{
Images: &tt.existing,
},
}
newImages := kustomizeOverride{
Kustomize: kustomizeImages{
Images: &tt.new,
},
}
expectedImages := kustomizeOverride{
Kustomize: kustomizeImages{
Images: &tt.expected,
},
}

mergeKustomizeOverride(&existingImages, &newImages)
assert.ElementsMatch(t, *expectedImages.Kustomize.Images, *existingImages.Kustomize.Images)
})
}
}