Clean up image values for horizon chart

[travertischio]

This PR cleans up how the image values are structured in the horizon chart

global is a section used to communicate values between charts. The container image horizon should not be overwritten by other charts. In addition the core exporter image is only used in one place

[jacekn]

It would be good to sync on future improvements for this chart but I think in the short term we should not make this change.
First of all it would make it harder to use this chart as subchart.
The second reason, which I think is even more important, is that we may have users out there who use existing values. If we rename values from underneath them we may break people's environments. Maybe we can sync and come up with a plan that includes version number changes, such as moving to chart 1.x.x to indicate breaking changes?

[jacekn]

The horizon image settings are in the global section to allow parent chart to override the value. There is an initial "stellar-network" chart somewhere that we planned to use to deploy full stacks for testing.
I think we should leave it as is

[travertischio]

a global chart can still overwrite the values that are not in the global section
however, putting values like this in the global section makes the chart less flexible, because any upstream values that might conflict would break a chart

[jacekn]

global is a section used to communicate values between charts. The container image horizon should not be overwritten by other charts.

The intention, from what I remember, was for the horizon image to be overwritten by the parent chart. I'll try to remember some more details.
Could you explain a bit more why you think the container image for horizon should not be overwriten?

In addition the core exporter image is only used in one place

It's used in 1 place in this chart. But the exporter is also used by the core chart

[jacekn]

We were planning on building full stellar network stacks and for that to work we needed the parent chart to be able to set the exporter. The exporter will also be used by the core chart which deploys validators.

[travertischio]

the parent chart can still set the exporter here

[travertischio]

First of all it would make it harder to use this chart as subchart.

As I wrote in the description this is actually the point of this move. global is the section that must be the same across subcharts, meaning if you have conflicting values if breaks subcharts. Parent charts can always specify values in other sections of subcharts

The second reason, which I think is even more important, is that we may have users out there who use existing values. If we rename values from underneath them we may break people's environments. Maybe we can sync and come up with a plan that includes version number changes, such as moving to chart 1.x.x to indicate breaking changes?

yea, right now the version is 0.0.7 which doesnt indicate that this is not even at a 1.0.0 yet
if were putting effort into this helm chart then we most likely need to clean it up and get to a 1.0.0 so we can follow semver better from there

[jacekn]

First of all it would make it harder to use this chart as subchart.

As I wrote in the description this is actually the point of this move. global is the section that must be the same across subcharts, meaning if you have conflicting values if breaks subcharts. Parent charts can always specify values in other sections of subcharts

I think the reason for using the global section may have been to remove duplication when deploying full stacks but it's been a while since I touched this code, I'll need to come back to it and double check.

But you raise some good points. If you can get things captured in a google doc we could collaborate on planning this refactoring taking all other components into accounts.

The second reason, which I think is even more important, is that we may have users out there who use existing values. If we rename values from underneath them we may break people's environments. Maybe we can sync and come up with a plan that includes version number changes, such as moving to chart 1.x.x to indicate breaking changes?

yea, right now the version is 0.0.7 which doesnt indicate that this is not even at a 1.0.0 yet if were putting effort into this helm chart then we most likely need to clean it up and get to a 1.0.0 so we can follow semver better from there

This chart was released so should be considered production code. While cleanup is very useful we should still do it in a controlled way. Breaking changes should include bump of a major version. Small cleanups, like in the other PRs you landed, are great to be done under 0.x.x of course.
But I think that reworking of the values file format is significant enough to warrant move from 0.x.x to 1.x.x. But before we do that I think we should get some more eyes on the problem and document the plan a bit better to ensure we minimize disruption