-
Notifications
You must be signed in to change notification settings - Fork 153
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
possibly depricate installplan-approver? #222
Comments
@pittar @zisisli @carlmes @PendaGTP @strangiato thoughts? |
Hey @itewk . I really like the new Helm-based approver. However, not everyone will be using Helm and there's a certain simplicity to referencing the existing approver in your Kustomize file. I agree that we should at a minimum reference the more robust Helm version in the README for the installplan-approver. I'll have to dig into your code a bit to see how much could be back ported into the simple Kustomize version. |
Agreed with @pittar! We should align them as best we can and provide both the helm and kustomize options. Lots of customers are very kustomize driven and avoid helm like it is the plague. Both the helm and kustomize patterns should be maintained as best as possible to support the different options. |
@pittar you could probably backport all of it into customize as well. we are then just maintinaing it in both palces. i went with helm because i wanted all teh approval stuff to be claned up after the fact, and helm hooks (which intigrate with argo hooks) are great for that. additionally, syncing parameters accross resources far easier with helm then kustomize. minimally you could take the bash in the new Job for the more advanced approval logic if nothing else. maybe leave some comment in both jobs in both repos that they are copies of eachothers logic so if we ever change one someone can go change the other. |
@pittar , @mertel-rh made a PR that ports the functionality from the helm chart (as best as possible) to this customize. |
@itewk , yes - looking at it now! Thanks! |
Over in the helm-charts repo we now have a super charged version of the installplan-approver from this repository that does things even more robustly. makign sure to only approve the correct installplan for instance.
maybe we should depricate the installplan-approver here and/or at least cross link from here to the more advanced helm chart?
https://github.com/redhat-cop/helm-charts/tree/master/charts/operators-installer
thoughts?
The text was updated successfully, but these errors were encountered: