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.

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

feat: update several path for image.tag #793

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Tchoupinax
Copy link

Hello folks!

I made this improvement to handle a case we have at work. Let's explain it.

In the current case, we provide a path for the image tag with the annotation .helm.image-tag. It says where to update the tag for this argoCD application.

Now, I have one ArgoCD application with two deployements, different with a suffix. Let's call them a and b. So my helm-chart values are prefixed by the alias of each app.

a:
  image:
    tag: v1.0.0

b:
  image:
    tag: v1.0.0

I want to update both at the same time, the lifecycle is identical. This merge request allows to do it, by adding several paths as value separated by a comma. Then, I can provide

xxxx.helm.image-tag=a.image.tag,b.image.tag

A unit test has been added about tag and name and moreover, I handled every space to ensure it won't have any bug for a mistype.

Finally, I build the customized image and tested, all is OK for me.
I hope all is clear!

Thank you for your work, I love this project.
Cheers!

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.92%. Comparing base (4f21ade) to head (175777e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #793      +/-   ##
==========================================
+ Coverage   74.82%   74.92%   +0.10%     
==========================================
  Files          31       31              
  Lines        3912     3928      +16     
==========================================
+ Hits         2927     2943      +16     
  Misses        850      850              
  Partials      135      135              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Tchoupinax <corentinfiloche@hotmail.fr>
Signed-off-by: Tchoupinax <corentinfiloche@hotmail.fr>
Signed-off-by: Tchoupinax <corentinfiloche@hotmail.fr>
@chengfang
Copy link
Collaborator

@Tchoupinax thanks for proposing it. I'll look into it.

@Tchoupinax
Copy link
Author

Hello @chengfang,
Could you have time to give a look again? 🙏

@chengfang chengfang added the enhancement New feature or request label Oct 21, 2024
@@ -427,12 +427,30 @@ func SetHelmImage(app *v1alpha1.Application, newImage *image.ContainerImage) err
mergeParams = append(mergeParams, p)
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason hpImageSpec is not updated to take multiple values too? Do we want to keep image.spec, image.name, and image.tag consistant in taking multi-path values?

@chengfang
Copy link
Collaborator

Some other places assume that image.name and image.spec represent single-path value, and we should think about if it makes sense for those places to also take multi-path values. For instance,

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

helmParamName := getHelmParam(appSource.Helm.Parameters, helmAnnotationParamName)

Annotations: map[string]string{
fmt.Sprintf(common.HelmParamImageNameAnnotation, "foobar"): "foobar.image.name,foobar2.image.name",
fmt.Sprintf(common.HelmParamImageTagAnnotation, "foobar"): "foobar.image.tag, foobar2.image.tag", // Space is expected
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

also need to handle cases like below to exclude empty slice elements caused by extra separators.
foobar.image.name,foobar2.image.name,
foobar.image.name,,,foobar2.image.name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants