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

askhari · 2024-05-27T13:55:29Z

It relates to PR #636 and includes a fix to set a default ImageAlias name on container objects in order to retrieve application's annotation values properly.

Also, this relates to the last comments from @zagr0 on issue #558.

This PR fixes a problem retrieving image-name and image-tag annotations when we try to update a Helm type application using the "helmvalues" way on the "write-back-target" annotation.
It includes a new GetImagesAndAliasesFromApplication function which sets the ImageAlias attribute for the containerImage object. This solves the problem retrieving annotations which uses the container alias name.

As with PR #636 , I tried to keep the same coding pattern and built the least invasive changes in the code flow. That's the reason of using a new function.
Also, there are test for all the new code, they cover the most basic cases.

Please give it a try and let's see if it can be merged. Any suggestions are more than welcome.

@jannfis , @pasha-codefresh , I'm not sure if this is the way to open a fix for release-0.13. What I did was the following:

Download the last changes.
Changed to release-0.13 branch.
Create a new branch fix/set-default-image-alias-with-helmvalues .
Create this PR to merge into release-0.13 .

Please, let me know if I need to work on this on any other way.

Signed-off-by: Pasha Kostohrys <pavel@codefresh.io>

fix: now using alias to retrieve annotations when updating Helm type app Signed-off-by: David Vidal Villamide <david@askharilabs.com>

jannfis · 2024-05-27T15:00:23Z

Hi @askhari, please submit all PRs against the default branch ("master"). All changes have to first go into the default branch, and maintainers will cherry-pick any changes to the release branches as required.

Thank you!

NissesSenap · 2024-05-27T15:13:52Z

I have tested this branch with one of our clusters, and it seems like this solves the issue with write-backs.
Thank you @askhari

askhari · 2024-05-27T15:42:23Z

@jannfis , thank you for the info.

Then, I'll make some ammendments and try to fix the integration test that is failing. this is due I branched from release-0.13 and it has that version in the kustomization.yaml and install.yml files.

askhari · 2024-05-27T16:36:57Z

@jannfis , I think it's all done.
I changed the base branch to be master and also changed the version for the docker image and pass the integration tests.
Again, thank you for the quick response :)

@NissesSenap , thank you for testing it so quickly.

Signed-off-by: David Vidal Villamide <david@askharilabs.com>

…e-updater into fix/set-default-image-alias-with-helmvalues

pasha-codefresh · 2024-05-29T08:29:07Z

pkg/argocd/update.go


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


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

Thank you for pointing it out :)

There are mainly two reasons for this decision:

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).

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.

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

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

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 :)

@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.

@askhari maybe you can help with manual test?

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.

Fine for me

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.

Again thank you for your help to solve this issue.

Cheers!

VERSION

Signed-off-by: David Vidal Villamide <david@askharilabs.com>

fix: use NormalizedSymbolicName() to retrieve name and version from annotations for Helm app types Signed-off-by: David Vidal Villamide <david@askharilabs.com>

…h-helmvalues' into fix/set-default-image-alias-with-helmvalues

…image for prevent code duplication Signed-off-by: Pasha Kostohrys <pavel@codefresh.io>

Signed-off-by: Pasha Kostohrys <pavel@codefresh.io>

jannfis

LGTM, thanks @askhari

pasha-codefresh

LGTM, thank you

askhari · 2024-06-05T16:17:21Z

Thank you very much @pasha-codefresh for your help and also to @jannfis and @chengfang for the approvals.

We usually delete branches after merging in our projects to keep it clean, so I'll proceed to delete it now.

pasha-codefresh and others added 2 commits May 15, 2024 10:54

Release v0.13.0

b40e984

Signed-off-by: Pasha Kostohrys <pavel@codefresh.io>

add: GetImagesAndAliasesFromApplication to retrieve images with aliases

00ce8a7

fix: now using alias to retrieve annotations when updating Helm type app Signed-off-by: David Vidal Villamide <david@askharilabs.com>

askhari mentioned this pull request May 27, 2024

Multiple helm sources: skipping app of type '' because it's not of supported source type #558

Open

askhari changed the base branch from release-0.13 to master May 27, 2024 15:14

askhari force-pushed the fix/set-default-image-alias-with-helmvalues branch from b09c8e0 to 00ce8a7 Compare May 27, 2024 15:28

changed: image version value to avoid issues with integration tests

43afe23

Signed-off-by: David Vidal Villamide <david@askharilabs.com>

askhari force-pushed the fix/set-default-image-alias-with-helmvalues branch from ccbdfb6 to 43afe23 Compare May 27, 2024 16:39

Merge branch 'master' of https://github.com/argoproj-labs/argocd-imag…

b86e898

…e-updater into fix/set-default-image-alias-with-helmvalues

pasha-codefresh requested changes May 29, 2024

View reviewed changes

VERSION Outdated Show resolved Hide resolved

askhari and others added 6 commits May 29, 2024 13:12

fix: revert back version

efec2b0

Signed-off-by: David Vidal Villamide <david@askharilabs.com>

change: make NormalizedSymbolicName() method public

26bf144

fix: use NormalizedSymbolicName() to retrieve name and version from annotations for Helm app types Signed-off-by: David Vidal Villamide <david@askharilabs.com>

Merge remote-tracking branch 'askhari/fix/set-default-image-alias-wit…

f84d0bc

…h-helmvalues' into fix/set-default-image-alias-with-helmvalues

fix: getHelmParamNamesFromAnnotation should take info from container …

bdcecc1

…image for prevent code duplication Signed-off-by: Pasha Kostohrys <pavel@codefresh.io>

optimize code

4a08ed6

Signed-off-by: Pasha Kostohrys <pavel@codefresh.io>

check if alias exist

f282927

Signed-off-by: Pasha Kostohrys <pavel@codefresh.io>

pasha-codefresh mentioned this pull request May 30, 2024

fix: git write back Helm value files #663

Open

chengfang approved these changes May 30, 2024

View reviewed changes

jannfis approved these changes May 31, 2024

View reviewed changes

Merge branch 'master' into fix/set-default-image-alias-with-helmvalues

c1784a3

pasha-codefresh approved these changes May 31, 2024

View reviewed changes

pasha-codefresh merged commit 3f47c8b into argoproj-labs:master May 31, 2024
10 checks passed

askhari deleted the fix/set-default-image-alias-with-helmvalues branch June 5, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

askhari commented May 27, 2024

jannfis commented May 27, 2024

NissesSenap commented May 27, 2024

askhari commented May 27, 2024

askhari commented May 27, 2024

pasha-codefresh May 29, 2024

askhari May 29, 2024

askhari May 29, 2024

pasha-codefresh May 29, 2024

askhari May 29, 2024

askhari May 29, 2024

pasha-codefresh May 30, 2024

askhari May 30, 2024 •

edited

pasha-codefresh May 30, 2024

askhari Jun 5, 2024

jannfis left a comment

pasha-codefresh left a comment

askhari commented Jun 5, 2024


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

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

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

Conversation

askhari commented May 27, 2024

jannfis commented May 27, 2024

NissesSenap commented May 27, 2024

askhari commented May 27, 2024

askhari commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askhari May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannfis left a comment

Choose a reason for hiding this comment

pasha-codefresh left a comment

Choose a reason for hiding this comment

askhari commented Jun 5, 2024

askhari May 30, 2024 •

edited