-
Notifications
You must be signed in to change notification settings - Fork 263
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
Support for multisource + upgrade k8s and argocd pkg #578
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Andre Ferraz <[email protected]>
Signed-off-by: Andre Ferraz <[email protected]>
Signed-off-by: Andre Ferraz <[email protected]>
a23580e
to
077debb
Compare
Thanks for your PR! Can you please take a look at the failing unit tests, as well as the failing DCO? The latter is usually fixed by signing off your commits (i.e. |
I am very interested in this feature. @deferraz could you give attention to the PR? thanks! |
hey any update on this PR? image updater is unusable if we use the argo terraform provider due to this. |
if app.Spec.Source.Kustomize == nil { | ||
app.Spec.Source.Kustomize = &v1alpha1.ApplicationSourceKustomize{} | ||
// This case seems not possible, but we check it anyway | ||
kustomizeSpec := app.Spec.GetSource().Kustomize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to fix the error showed in the unit tests I should change some code back to the original and leave it like the suggestion below where the app.Spec.Source.Kustomize will be initialized:
kustomizeSpec := app.Spec.GetSource().Kustomize | |
app.Spec.Source.Kustomize = app.Spec.GetSource().Kustomize | |
if app.Spec.Source.Kustomize == nil { | |
app.Spec.Source.Kustomize = &v1alpha1.ApplicationSourceKustomize{} | |
} | |
for i, kImg := range app.Spec.Source.Kustomize.Images { | |
curr := image.NewFromIdentifier(string(kImg)) | |
override := image.NewFromIdentifier(ksImageParam) | |
if curr.ImageName == override.ImageName { | |
curr.ImageAlias = override.ImageAlias | |
app.Spec.Source.Kustomize.Images[i] = v1alpha1.KustomizeImage(override.String()) | |
} | |
} | |
app.Spec.Source.Kustomize.MergeImage(v1alpha1.KustomizeImage(ksImageParam)) | |
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this snippet to fix the errors showed by the unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tried this and it worked to some degree. But in the end, if you want to set the target writeback repository is a bit messy. It would be better to use an annotation.
There is already a merge in the master branch of this repository that adds an annotation to set the writeback repository using multisource, but it seems to fail in the unit tests.
Digging a bit into the code, for this to work properly, it will need some dependencies upgrades (specially the argocd depedency) and a much deeper refactor and development to add this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a new PR here (#636) with similar changes to this one but covering some issues that we found when we wanted to write back to Helm values files. I hope this helps.
Please, let me know if we should merge this to PR contents to have only one and work on that.
Would be awesome to get this approved |
Agree, we are looking for this feature too. |
Any news about this PR, we really need it |
Definitely need this on my team. |
bump? need to add in the next release! |
1 similar comment
bump? need to add in the next release! |
I think this now has been superseded by #636 |
This PR splits the necessary changes to have multisource support as requested by @jannfis
References:
#548
#564
Fixes #513