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

Fix/helm values wbc #748

Merged
merged 10 commits into from
Jun 24, 2024
Merged

Conversation

askhari
Copy link
Contributor

@askhari askhari commented Jun 16, 2024

This PR relates to the previous PRs, issues and comments:

These changes solve the following issues:

  1. does not search into the YAML hierarchy (Git write-back-target helmvalues does not replace tag and repo in the image section #744 ): now this feature will search for the key name on the root node of the YAML file. If it doesn't exist, then it will navigate the YAML hierarchy to look for the right key. If it does not find the key, then it will fail.
  2. Currently it messes up the order of the keys in the YAML: with these changes, we got rid of the Conflate library and use a yaml.MapSlice object instead. This way we can preserve the order and avoid messing up the YAML order when writing back to the repository.

To preserve retrocompatibility in case someone already use dots in the YAML keys, we first look for the complete key with dots. Then, if it's not found we go through the YAML hierarchy to find the key.

For example, using an annotation like this: argocd-image-updater.argoproj.io/labcat.helm.image-tag: rollout.image.tag; first we'll try to find a key named "rollout.image.tag" in the YAML root node. And if we do not find it, then it will search the YAML hierarchy searching the following structure:

rollout:
  image:
    tag: <your preferred value>

There are new tests for these code.

Please, comments or suggestions are more than welcome.

Cheers!

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.20%. Comparing base (a309127) to head (767175c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #748      +/-   ##
==========================================
+ Coverage   55.71%   56.20%   +0.48%     
==========================================
  Files          31       31              
  Lines        3060     3094      +34     
==========================================
+ Hits         1705     1739      +34     
  Misses       1218     1218              
  Partials      137      137              

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

Signed-off-by: David Vidal Villamide <[email protected]>
go.mod Show resolved Hide resolved
// If we're at the final key, set the value
current[j].Value = value
found = true
break
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just return here since we've set the value and nothing else needs to be done?

@alexismaior
Copy link

alexismaior commented Jun 22, 2024

I tested locally with a similar configuration and it worked like a charm! Thanks for working on this.

@chengfang chengfang merged commit 38c82f3 into argoproj-labs:master Jun 24, 2024
9 checks passed
@chengfang
Copy link
Collaborator

Thanks @askhari for the fix, and thanks @alexismaior for verifying it!

@seagyn
Copy link

seagyn commented Jul 4, 2024

Thanks for this fix. We've just run into the issue, is there an expected ETA for a new release of image updater or an image we can use in the mean time?

@pasha-codefresh
Copy link
Collaborator

I am going to create release on Monday

@seagyn
Copy link

seagyn commented Jul 4, 2024

I am going to create release on Monday

Appreciate it!

sribiere-jellysmack pushed a commit to sribiere-jellysmack/argocd-image-updater that referenced this pull request Aug 13, 2024
Signed-off-by: David Vidal Villamide <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants