-
Notifications
You must be signed in to change notification settings - Fork 265
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: git write back Helm value files #663
Conversation
Oh I'm cheering for this. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #663 +/- ##
==========================================
+ Coverage 57.03% 57.10% +0.07%
==========================================
Files 30 30
Lines 2979 3003 +24
==========================================
+ Hits 1699 1715 +16
- Misses 1145 1152 +7
- Partials 135 136 +1 ☔ View full report in Codecov by Sentry. |
Hi @burnb - I really appreciate your work on this. I tried this out this morning and unfortunately still ran into the error |
Hi @devopsidiot could you show me what kind of annotation did you try to execute? |
Hi @burnb - Here you go (and thanks for responding):
|
helm.image-name and helm.image-tag are the paths to the values which will be generated by updater in the helmvalues file, in your case in the values-rnd.yaml
and it will be represented in values-rnd.yaml after updater refresh like image:
name: <accountid>.dkr.ecr.us-east-1.amazonaws.com/daily-worship
tag: develop-v0.20.0-33 |
So I did the things correctly, but it's still not updating the application:
Annotations:
Relevant values:
|
in your case annotation should be
|
Thanks so much for responding: I did these things:
And the only error that I got was Since these argo applications are in the argo namespace and not the argocd namespace, am I just SoL for the time being? |
It's just a warning that it can't send event to the k8s. And at this step it successfully sent git update. |
You literally made me cry with relief. Thank you so much for all your hard work. This needs to get adopted. This makes it work. |
@burnb you are a hero! Finally ArgoCD somewhat resembles the power of Flux |
Hey, I have been using this and it works great. I am cheering for this too. I can attest that it works when using aliases in the image list. It writes to a specified file as long as the directory of the file exists. The one piece that seemed to be missing is the ability to write to a directory that does not exist yet. I opened a separate PR for it since it does not directly relate to your PR, but if the maintainers prefer, they can combine our PRs. |
@burnb Could you take a minute and give me a hint too? Could not update application spec: could not find an image-name annotation for image emm/arm Annotations: argocd-image-updater.argoproj.io/image-list: arm=docker-registry.<name>/emm/arm
argocd-image-updater.argoproj.io/arm.helm.image-name: deployment.arm.image.repository
argocd-image-updater.argoproj.io/arm.helm.image-tag: deployment.arm.image.tag
argocd-image-updater.argoproj.io/write-back-method: git
argocd-image-updater.argoproj.io/write-back-target: "helmvalues:values_dev.yaml" Values: ...
.
deployment:
arm:
image:
repository: docker-registry.<name>/emm/arm
tag: "9.0-1" If I remove the helm:
parameters:
- name: deployment.arm.image.repository
value: docker-registry.<name>/emm/arm
forcestring: true
- name: deployment.arm.image.tag
value: 9.0-57
forcestring: true If I add the Just in case, tree looks like this: helm/
├── emm-helm
│ ├── values.yaml
│ ├── values_dev.yaml
│ ├── ... I will be eternally grateful if you have time to help with this! |
@s-ledyakhov I guess the problem is with parsing of your image link by this function: func ParseNormalizedNamed(s string) (Named, error) {
if ok := anchoredIdentifierRegexp.MatchString(s); ok {
return nil, fmt.Errorf("invalid repository name (%s), cannot specify 64-byte hexadecimal strings", s)
}
domain, remainder := splitDockerDomain(s)
var remote string
if tagSep := strings.IndexRune(remainder, ':'); tagSep > -1 {
remote = remainder[:tagSep]
} else {
remote = remainder
}
if strings.ToLower(remote) != remote {
return nil, fmt.Errorf("invalid reference format: repository name (%s) must be lowercase", remote)
}
ref, err := Parse(domain + "/" + remainder)
if err != nil {
return nil, err
}
named, isNamed := ref.(Named)
if !isNamed {
return nil, fmt.Errorf("reference %s has no name", ref.String())
}
return named, nil
} from the "github.com/distribution/distribution/v3/reference" package, coz this error doesn't go to the logs and in that case, it parses image a bit differently, so alias doesn't go anywhere, and this is the root of the problem - it operates with the image name. |
Thanks for the answer |
I think the problem is with your |
No, I checked this point |
I can confirm this PR fixes the issues with nested image parameters, however, it does rewrite the value file to sort all yaml/helm keys alphabetically. was this introduced in this PR or in #636? Can we avoid re-arranging helm parameters? |
@mubarak-j fixed, but I'm not quite sure about why linter alerting, I can't reproduce it locally and all the messages not about the PR code. |
Hi, can the fix be moved forward to be merged? |
3900089
to
e1646e4
Compare
@mubarak-j could you please resolve conflicts ? |
@burnb can you please do the honor? 🙏🏼 IMHO I think it's worth adding an example in the docs for using |
Left comment #651 (review) |
@burnb can you please take another look, rebase and resolve conflicts based on recent review comments? Thanks! |
@chengfang i think we should merge this one first #725 |
Signed-off-by: Cyril MARIN <[email protected]> Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
… file order Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
Signed-off-by: Aleksandr Petrov <[email protected]>
Done, I hope didn't break anything, coz some of the fixes was merged and done a bit differently. |
@burnb thanks for your continued work on this important issue. Unfortunately, there are still conflicts due to some recent merges. I took a look and jotted down a few notes below. Would you be able to take another look, resolve the manual merges?
|
Does the recently merged #748 implement the same fix as this PR? |
@chengfang Now I don't see any reason to go further with my RP and IMHO it looks weird that PR's merge goes out of order so PR made half year ago become useless because someone did double work 2 weeks ago and PR was quickly merged. |
@burnb sorry for being disorganized when merged PRs, and thanks for your deep investigation and the fixes. Even though this PR cannot be merged, it contributes a great deal towards resolving the issue. Thanks to everyone here for the great discussion and insights. |
Related: #650
Fixes for #636
Based on PR #651