fluxcd
Added custom yaml anchors
Enhancement
- Added custom anchor syntax for simple string substitution
First, I know this project is deprecated. This will help me get off of it. I have to unify multiple platforms between onprem and the cloud. This gives me some flexibility to do that. Once things are stable I can start the push to flux v2. I don't have the time or bandwidth to make a bunch of architectural changes right now when this serves my needs.
I made every attempt to follow all the guidelines, I intentionally made separate methods that have no impact on existing ones. If you don't pass an anchor string, absolutely nothing happens or changes. You have a method that walks the values map, I could have integrated there, I intentionally chose not to. Same with tests. I could have integrated into the struct and flow you had, but I made another (very similar) method that tests just my addition (both success and failure).
The code is clean. It's a single pass, with some strings being swapped out using references. Overall I'm very happy with this. I added documentation etc. I think it's a lot of value for very little risk and it's my first attempt at contribution to a major project so I'm hoping we can merge this
Thank you for sending this. I'm going to take some time to review the change that you sent, with extra attention because you said this is something that will help you migrate to helm-controller.
I read in a separate issue report that e2e is having issues, so I don't expect that whatever failure we're seeing from CircleCI is something caused by your change.
I did sneak a fix in, but was the doc string for my method had multiple typos. Programmatically it would have be inconsequential
@kingdonb I don't know if you had a chance to look at this. There was an issue my unit test didn't catch. Something to do with the way reflect would try to cast helm values. Even though helm.Values and map[string]interface{} share the same base type it would panic somewhere down the recursion. Now I just just use reflects native MapRange() to walk the values and it works as intended. I also added some more tests for nil. I'll take a look at that e2e stuff tomorrow too if you haven't had a chance yet
I haven't had much time for Helm Operator to be blunt but I am coming back around to it, thank you for sending this and continuing to work at it. 🥁
The merge checks are passing, and that's about all I can say from the review I've given it.
I know there are some other folks asking for Helm Operator release with the change to point at the new charts.helm.sh repo, for example, I will be happy to manage a release for Helm Operator if the timing is right for one. I'm not sure, haven't discussed whether or if there are other changes that we were waiting to include.
I unfortunately don't think that the disposition of this is going to be to merge this PR.
Simply by reasoning that it is a new feature and it changes something substantial about how Helm Operator works, and we are in maintenance mode which really is meant to put the brakes on new features quite a bit.
I will confer with project maintainers and give some time for reconsidering, but I don't see this as something within the scope of maintenance and I'm not sure if it can actually be merged while maintaining in good faith with the declaration that Flux v1 and Helm Operator are in maintenance mode. It seems too much like a new feature.
@kingdonb For sure, it is a feature but I have to get my current setup to an end state before I can try to affect change in our other clusters within our organization and this will help me do that.
But question is this then though did we confer with the community that they wanted the project in maintenance? Who drove this need to deprecate the project? You have pulls coming in that are languishing and a community that seems happy with the functionality. I know our cloud team uses terraform to install flux with helm. There's a helm terraform provider. I'm sure they can switch to bash and use the cli utility to do installs on top of which ever other changes there are. Already v2 is moving away from community standard interfaces for installation at least.
I can fork and then start merging in other peoples pulls and stuff. Like heck I'd be willing to maintain a major fork if I have too, but then the community splits or doesn't etc. Maybe if people find the new fork they'll use it. I guess if people still are interested in v1 you should give it a new home.
The need for a rewrite was determined by consensus of the maintainers, who answered many support requests and thoughtfully consider them as input to make a better version that can be supported forward.
The need for breaking changes was decided by inheritance, since so much of the function of Helm Operator is driven by Helm upstream, there came to a point where it was determined "we can either end Helm 2 support, and break some people's workflow, or let them upgrade to the next version and put any breaking changes that are needed over there."
This was the approach that was determined to be more likely to result in less groaning overall, I think.
So, it's basically a feature for adding custom YAML anchors.
This is really interesting now that I've had some time to look at it, and I think I'm more curious about why this is something you needed in a release, than I am about anything else.
I just found some YAML anchors in a chart I wanted to use, in some examples for a helm values file. I think probably I knew this was part of the YAML spec, but I didn't know if it was supported by Helm or Helm Operator, or any other gitops tools, (but I guess as it is part of the YAML spec, it would be supported by all of those.)
I wondered enough to ask if custom yaml anchors are then supported by any of these other tools?
You have gone to the trouble of making sure it would be a non-breaking change, so that is one point in favor of merging. If it is something that you will actually use, and you are a user already who is stuck on Helm Operator for a while, I think that's another point in favor. Given that you also added tests, it's ultimately very hard to turn your contribution away!
But if this feature is not supported in Helm Controller or any other tools, I have a harder time understanding how this will help you migrate away from Helm Operator.
I understand you've expressed an interest in forking and/or maintaining a fork. This is not something which can be prohibited and we appreciate your help with supporting Flux and Helm. Forks and splits are not usually in the best interest of the community in my experience, it is better if the community can stay together and keep contributions going toward one sink.
@kingdonb
So helm actually does support native yaml anchors out of the box. There's a handful of problems along the way though for the operator to manage that
Getting anything into kubernetes with intact anchors is impossible. Since the yaml library marshals everything into json first any * comes out as a string. So you can't have anchors in config maps etc. Same issue with the actual helm release object. You also see issues trying to get anchors through customize. So since there's multiple levels of abstraction ( its yaml all the way down lol) by the time it gets to the operator, native yaml anchors don't work.
As for how it helps me migrate. So right now, we have a team of cloud engineers that stood up a platform around EKS. We use flux to manage that fleet. I was tasked with "Take all this, and make it work on prem". The big thing this gives me is flexibility. Once we have a unified repo that manages both cloud and on prem, from here I can then move to change the state of the overarching ecosystem. But I can't go right to v2 without bringing the cloud team with me. It doesn't matter so much that the future thing supports the anchors just that I can more easily get everything to a same state quicker.
So helm actually does support native yaml anchors out of the box
I was asking about custom yaml anchors, which is a solution I've never seen before, and I think from your description I understand why it's needed – you can't use YAML anchors in HelmReleaseSpec.Values without them being reified too early.
Will this need a corresponding change in Helm Controller to support the same behavior there, so that it can be used the same in Flux v1 and Helm Operator, as in the Helm Controller of Flux v2?
I think there might be less radical solutions that already can work now, without changes to either Operator or Controller, that will make your situation easier. I think the type of indirection you're trying to do is much easier with a solution like Jsonnet, which you can use to prepare manifests in advance of them being applied.
I've spent some time preparing fluxcd/flux2#1200 with a tutorial on GitHub Actions and Jsonnet, but it does not address your specific question with an example (partially because I'm not exactly clear on what problems you need YAML anchors to help solve.) A language like jsonnet can do many things with indirection that Kustomize or plain YAML cannot easily approach.
The disposition of this PR is strictly not up to me, but I can try to advocate it for you to the developers. I definitely wouldn't count on it getting merged and released as an upstream release quickly. That of course doesn't mean you can't build and use it for your own purposes.
If this seems like too much overhead, please understand this is a CNCF project which has to stand up to design scrutiny and oversight from the community. Decisions about functional direction of the project cannot be taken likely, and must be made intentionally and with future planning kept in mind. ✊ 🤝
Thanks for contributing here! We are not going to make any changes like this here, Flux v1 and Helm Operator are in maintenance mode
Closing, stale.
Sorry for the delay in response, if you have questions welcome to reopen this or a new issue.