Skip to content
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

fix: set default ImageAlias for Helm app type using helmvalues git write-back method #725

Merged
merged 11 commits into from
May 31, 2024
Merged

fix: set default ImageAlias for Helm app type using helmvalues git write-back method #725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b40e984
Release v0.13.0
pasha-codefresh May 15, 2024
00ce8a7
add: GetImagesAndAliasesFromApplication to retrieve images with aliases
askhari May 27, 2024
43afe23
changed: image version value to avoid issues with integration tests
askhari May 27, 2024
b86e898
Merge branch 'master' of https://github.com/argoproj-labs/argocd-imag…
pasha-codefresh May 29, 2024
efec2b0
fix: revert back version
askhari May 29, 2024
26bf144
change: make NormalizedSymbolicName() method public
askhari May 29, 2024
f84d0bc
Merge remote-tracking branch 'askhari/fix/set-default-image-alias-wit…
pasha-codefresh May 29, 2024
bdcecc1
fix: getHelmParamNamesFromAnnotation should take info from container …
pasha-codefresh May 29, 2024
4a08ed6
optimize code
pasha-codefresh May 29, 2024
f282927
check if alias exist
pasha-codefresh May 29, 2024
c1784a3
Merge branch 'master' into fix/set-default-image-alias-with-helmvalues
pasha-codefresh May 31, 2024
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
31 changes: 23 additions & 8 deletions pkg/argocd/argocd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,29 +309,26 @@ func (client *argoCD) UpdateSpec(ctx context.Context, in *application.Applicatio
// getHelmParamNamesFromAnnotation inspects the given annotations for whether
// the annotations for specifying Helm parameter names are being set and
// returns their values.
func getHelmParamNamesFromAnnotation(annotations map[string]string, symbolicName string) (string, string) {
func getHelmParamNamesFromAnnotation(annotations map[string]string, img *image.ContainerImage) (string, string) {
// Return default values without symbolic name given
if symbolicName == "" {
if img.ImageAlias == "" {
return "image.name", "image.tag"
}

var annotationName, helmParamName, helmParamVersion string

// Image spec is a full-qualified specifier, if we have it, we return early
annotationName = fmt.Sprintf(common.HelmParamImageSpecAnnotation, symbolicName)
if param, ok := annotations[annotationName]; ok {
if param := img.GetParameterHelmImageSpec(annotations); param != "" {
log.Tracef("found annotation %s", annotationName)
return strings.TrimSpace(param), ""
}

annotationName = fmt.Sprintf(common.HelmParamImageNameAnnotation, symbolicName)
if param, ok := annotations[annotationName]; ok {
if param := img.GetParameterHelmImageName(annotations); param != "" {
log.Tracef("found annotation %s", annotationName)
helmParamName = param
}

annotationName = fmt.Sprintf(common.HelmParamImageTagAnnotation, symbolicName)
if param, ok := annotations[annotationName]; ok {
if param := img.GetParameterHelmImageTag(annotations); param != "" {
log.Tracef("found annotation %s", annotationName)
helmParamVersion = param
}
Expand Down Expand Up @@ -503,6 +500,24 @@ func GetImagesFromApplication(app *v1alpha1.Application) image.ContainerImageLis
return images
}

// GetImagesFromApplicationImagesAnnotation returns the list of known images for the given application from the images annotation
func GetImagesAndAliasesFromApplication(app *v1alpha1.Application) image.ContainerImageList {
images := GetImagesFromApplication(app)

// We update the ImageAlias field of the Images found in the app.Status.Summary.Images list.
for _, img := range *parseImageList(app.Annotations) {
if image := images.ContainsImage(img, false); image != nil {
if img.ImageAlias == "" {
image.ImageAlias = img.ImageName
} else {
image.ImageAlias = img.ImageAlias
}
}
}

return images
}

// GetApplicationTypeByName first retrieves application with given appName and
// returns its application type
func GetApplicationTypeByName(client ArgoCD, appName string) (ApplicationType, error) {
Expand Down
83 changes: 77 additions & 6 deletions pkg/argocd/argocd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,65 @@ func Test_GetImagesFromApplication(t *testing.T) {
})
}

func Test_GetImagesAndAliasesFromApplication(t *testing.T) {
t.Run("Get list of images from application", func(t *testing.T) {
application := &v1alpha1.Application{
ObjectMeta: v1.ObjectMeta{
Name: "test-app",
Namespace: "argocd",
},
Spec: v1alpha1.ApplicationSpec{},
Status: v1alpha1.ApplicationStatus{
Summary: v1alpha1.ApplicationSummary{
Images: []string{"nginx:1.12.2", "that/image", "quay.io/dexidp/dex:v1.23.0"},
},
},
}
imageList := GetImagesAndAliasesFromApplication(application)
require.Len(t, imageList, 3)
assert.Equal(t, "nginx", imageList[0].ImageName)
assert.Equal(t, "that/image", imageList[1].ImageName)
assert.Equal(t, "dexidp/dex", imageList[2].ImageName)
})

t.Run("Get list of images and image aliases from application that has no images", func(t *testing.T) {
application := &v1alpha1.Application{
ObjectMeta: v1.ObjectMeta{
Name: "test-app",
Namespace: "argocd",
},
Spec: v1alpha1.ApplicationSpec{},
Status: v1alpha1.ApplicationStatus{
Summary: v1alpha1.ApplicationSummary{},
},
}
imageList := GetImagesAndAliasesFromApplication(application)
assert.Empty(t, imageList)
})

t.Run("Get list of images and aliases from application annotations", func(t *testing.T) {
application := &v1alpha1.Application{
ObjectMeta: v1.ObjectMeta{
Name: "test-app",
Namespace: "argocd",
Annotations: map[string]string{
common.ImageUpdaterAnnotation: "webserver=nginx",
},
},
Spec: v1alpha1.ApplicationSpec{},
Status: v1alpha1.ApplicationStatus{
Summary: v1alpha1.ApplicationSummary{
Images: []string{"nginx:1.12.2"},
},
},
}
imageList := GetImagesAndAliasesFromApplication(application)
require.Len(t, imageList, 1)
assert.Equal(t, "nginx", imageList[0].ImageName)
assert.Equal(t, "webserver", imageList[0].ImageAlias)
})
}

func Test_GetApplicationType(t *testing.T) {
t.Run("Get application of type Helm", func(t *testing.T) {
application := &v1alpha1.Application{
Expand Down Expand Up @@ -541,7 +600,9 @@ func Test_GetHelmParamAnnotations(t *testing.T) {
fmt.Sprintf(common.HelmParamImageSpecAnnotation, "myimg"): "image.blub",
fmt.Sprintf(common.HelmParamImageTagAnnotation, "myimg"): "image.blab",
}
name, tag := getHelmParamNamesFromAnnotation(annotations, "")
name, tag := getHelmParamNamesFromAnnotation(annotations, &image.ContainerImage{
ImageAlias: "",
})
assert.Equal(t, "image.name", name)
assert.Equal(t, "image.tag", tag)
})
Expand All @@ -551,7 +612,9 @@ func Test_GetHelmParamAnnotations(t *testing.T) {
fmt.Sprintf(common.HelmParamImageSpecAnnotation, "myimg"): "image.path",
fmt.Sprintf(common.HelmParamImageTagAnnotation, "myimg"): "image.tag",
}
name, tag := getHelmParamNamesFromAnnotation(annotations, "myimg")
name, tag := getHelmParamNamesFromAnnotation(annotations, &image.ContainerImage{
ImageAlias: "myimg",
})
assert.Equal(t, "image.path", name)
assert.Empty(t, tag)
})
Expand All @@ -561,7 +624,9 @@ func Test_GetHelmParamAnnotations(t *testing.T) {
fmt.Sprintf(common.HelmParamImageNameAnnotation, "myimg"): "image.name",
fmt.Sprintf(common.HelmParamImageTagAnnotation, "myimg"): "image.tag",
}
name, tag := getHelmParamNamesFromAnnotation(annotations, "myimg")
name, tag := getHelmParamNamesFromAnnotation(annotations, &image.ContainerImage{
ImageAlias: "myimg",
})
assert.Equal(t, "image.name", name)
assert.Equal(t, "image.tag", tag)
})
Expand All @@ -571,7 +636,9 @@ func Test_GetHelmParamAnnotations(t *testing.T) {
fmt.Sprintf(common.HelmParamImageNameAnnotation, "otherimg"): "image.name",
fmt.Sprintf(common.HelmParamImageTagAnnotation, "otherimg"): "image.tag",
}
name, tag := getHelmParamNamesFromAnnotation(annotations, "myimg")
name, tag := getHelmParamNamesFromAnnotation(annotations, &image.ContainerImage{
ImageAlias: "myimg",
})
assert.Empty(t, name)
assert.Empty(t, tag)
})
Expand All @@ -580,14 +647,18 @@ func Test_GetHelmParamAnnotations(t *testing.T) {
annotations := map[string]string{
fmt.Sprintf(common.HelmParamImageTagAnnotation, "myimg"): "image.tag",
}
name, tag := getHelmParamNamesFromAnnotation(annotations, "myimg")
name, tag := getHelmParamNamesFromAnnotation(annotations, &image.ContainerImage{
ImageAlias: "myimg",
})
assert.Empty(t, name)
assert.Equal(t, "image.tag", tag)
})

t.Run("No suitable annotations found", func(t *testing.T) {
annotations := map[string]string{}
name, tag := getHelmParamNamesFromAnnotation(annotations, "myimg")
name, tag := getHelmParamNamesFromAnnotation(annotations, &image.ContainerImage{
ImageAlias: "myimg",
})
assert.Empty(t, name)
assert.Empty(t, tag)
})
Expand Down
10 changes: 8 additions & 2 deletions pkg/argocd/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,10 +416,16 @@ func marshalParamsOverride(app *v1alpha1.Application, originalData []byte) ([]by
}

if strings.HasPrefix(app.Annotations[common.WriteBackTargetAnnotation], common.HelmPrefix) {
images := GetImagesFromApplication(app)
images := GetImagesAndAliasesFromApplication(app)

for _, c := range images {
helmAnnotationParamName, helmAnnotationParamVersion := getHelmParamNamesFromAnnotation(app.Annotations, c.ImageName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why you need to change this logic? why just not replace it with c.ImageAlias if it is exist ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing it out :)

There are mainly two reasons for this decision:

  1. The code of getHelmParamNamesFromAnnotation it's only used here and anywhere else in the code. I used it at first because it was convenient at the time, but it seems that it may be deprecated if it has no use anymore (I was going to suggest that in another issue later on).
  2. Using GetParameterHelmImageName and GetParameterHelmImageTag seems a better option because this two functions uses normalizedSymbolicName function internally, which normalizes the ImageAlias enabling its use in annotations properly because it replaces the "/" character for "_".

If in the end it should be better to leave it as before, just let me know and I'll make revert the changes to use c.ImageAlias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot... please, if in the end no changes should be done, let me know to mark the conversation as resolved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just concerned that you actually changing logic, because inside getHelmParamNamesFromAnnotation here more conditions. I would not change it for now. Also this function is well covered with tests, in case if you are going to use new logic, you have to cover it with unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, you are absolutely right and I should think thrice before changing things.

I'll change it back to the previous statement changing the c.ImageName for c.normalizedSymbolicName() which will work properly with the annotation names because it already replaces the "/" characters for "_". I'll check if the logic works that way and push the changes back.

Thank you for noticing and explaining it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pasha-codefresh , thank you very much for your help. I really appreciate it and with those changes I now understand what you meant by passing the container image. Thank you!

Do you need me to work on some more changes from my side for this PR? I think that with the changes you made, the problem is already solved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@askhari maybe you can help with manual test?

Copy link
Contributor Author

@askhari askhari May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure! I'll run it on a cluster where we have a test application just for this purpose and will paste here the results to show the validation.
It will take a few days because I'll be off and I'm not sure if I'll have enough time today to test it properly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that this is already merged. Thank you for your help @pasha-codefresh .

Still, I attach a test I made to validate that the code is retrieving the information properly when using aliases.
Below there is a screenshot which uses an application definition with a docker image called labduck and using an alias labbat to validate that it is retrieving the values properly from the annotations.

Captura de pantalla 2024-06-05 a las 17 59 27

Again thank you for your help to solve this issue.

Cheers!


if c.ImageAlias == "" {
continue
}

helmAnnotationParamName, helmAnnotationParamVersion := getHelmParamNamesFromAnnotation(app.Annotations, c)

if helmAnnotationParamName == "" {
return nil, fmt.Errorf("could not find an image-name annotation for image %s", c.ImageName)
}
Expand Down
Loading