Fix quoting of password etc in helm-operator chart

[dimbleby]

Values in the generated repositories.yaml are now correctly quoted

Fixes #629.

Output from the values.yaml given in that issue is now:

apiVersion: v1
generated: 0001-01-01T00:00:00Z
repositories:
- name: fluxcd
  url: https://charts.fluxcd.io
  cache: /var/fluxd/helm/repository/cache/fluxcd-index.yaml
  caFile: ""
  certFile: ""
  keyFile: ""
  password: "this-\\-is-a-literal-backslash"
  username: "dimbleby"

which is correctly escaped

[kingdonb]

Thanks for your contribution. I understand your position that this is a bug that requires a workaround, and it's desirable to not have the workaround anymore.

The question that I have for you now, with that out of the way, and with a mind toward merging your change...

If you upgrade the operator, leaving your workaround in place (please forget that it is desirable to get rid of this for now), does it change the behavior in a way that requires a config update, or does everything continue working same as before?

From conversations with the Flux maintainer team, I can say the sentiment I've heard repeated around how to tell a breaking change, is that if it's a behavior change, then it is a breaking change, and it is not desirable to promote any breaking changes while the project is in maintenance unless they are critical fixes that would potentially prevent Flux from being used anymore. This includes updates that resolve some work-around in a way that requires affected users to read the Changelog carefully and make some modifications to their configuration, so that it doesn't break upon upgrade.

The commitment that was expressed to Flux users was that we would honor semver and not make any breaking changes, so that teams who are stuck for now on the legacy Flux v1 and Helm Operator will be able to upgrade and receive security patches and critical bugfixes without raising their level of investment to maintain and keep using the technology that has been marked as EOL. While this is a guideline and rules are not meant to be applied blindly without consideration, it's imperative that we maintain that commitment (unless there's a really strong argument to break it.)

Users who adopt Flux v2 while the version number is 0.x can expect that while they are upgrading from one minor release to the next, they can contain breaking changes and they must read the changelog carefully; users who adopt a GA product with a v1 semver commitment will usually have a different expectation, that v1 means v1 and as long as semver is being honored, they can upgrade regularly and go on doing whatever supported behavior they've been doing, without necessarily paying close attention to the upgrades, and without parsing every detail in every changelog for every new release, until there is a MAJOR increment.

If this is a breaking change, since it is only in the chart, we could potentially increment the major version of the chart itself, but I have a feeling this would not be considered by the maintainers as we are trying to discourage individual adopters from staying on Flux v1 forever. There are some issues that cause discomfort which cannot be addressed without breaking changes, and for those issues we started the Flux2 project (with the intention being to strictly honor semver.)

[kingdonb]

With all that out of the way, I feel like using the |quote function provided in sprig to properly quote seems like a no-brainer change and I would be in favor of merging it if I can get agreement from the maintainers.

[dimbleby]

Yes, this would be breaking for users who:

• are using a password that contains a backslash
• have understood that configuring their Helm chart to use this password does not work as expected
• have worked around this by adding an extra backslash to their password in the Helm chart values

Then after fixing this bug they would find that they need to remove the extra backslash.

I understand what you're saying, and agree that if it weren't for the awkwardness of deciding whether this "counts" as breaking then it looks like a no-brainer fix.

I hope that you are able to get this merged but if not: I appreciate you taking the time to think about this and to make the difficulty clear.

I guess I would note that all bug fixes change behaviour one way or another - else they wouldn't fix anything...

[kingdonb]

Thank you @dimbleby for engaging this discussion as well, since if we are to merge the change indeed, it will help to have these notes which lay clear and make explicit which type of users are affected. 👍

[kingdonb]

Closing, stale.

Sorry for the delay in response, if you have questions welcome to reopen this or a new issue.