[velero] Chart ux improvements

[kav]

Addressing the Chart UX issues filed previously

• existingSecretKey now specifies the key inside the existingSecret containing the IAM credentials. This actually also controls a generated secret in this case.
• The plugin container is now automatically added to initContainers based on all of the provider value. This can be overridden a number of ways, most simply by changing properties of the pluginImage key. I did guess on the alicloud config so if someone could verify that I'd appreciate it. May end up with extra initContainers if using "old" values.
• deployRestic is now restic.enabled Fully supports back compat.
• useSecret is now computed based on the existence of secret contents or the existing secret.
• Removed configuration wrapper, added tests to ensure behavior with or without wrapper.
• schedules is now an array with a name property in each entry. Also with back compat
• ~~Started on adding a values table to the readme as is standard in most helm charts.~~

As an aside: Not entirely sure I grok the credentials key. Prior to these change it appears the credentials.secretContents and credentials.extraEnvars ends up injected as both files and envvars and the deployments are looking for a key called cloud but that isn't specified in the docs. Perhaps there is velero cli context I don't have here.

Fixes #88 Fixes #89 Fixes #90 Fixes #139

[kav]

There are probably a few additional UX clean up tasks we should roll in if we are revving as a breaking change before we merge. I'll try to get at least a single proposal issue with a list filed shortly for discussion. I'd like to clean up the readme so a new user knows exactly which values they must provide for a minimum functional deployment.

[carlisia]

/rebase

[kav]

Also any open questions and changes for https://github.com/vmware-tanzu/helm-charts/issues/139 should be resolved before this goes to main. Are there other breaking changes under discussion? I can start a major version branch if we've got other stuff to roll together

[kav]

Also any open questions and changes for #139 should be resolved before this goes to main. Are there other breaking changes under discussion? I can start a major version branch if we've got other stuff to roll together

Switched version to 3.0.0-alpha.1 so if we do merge and have additional breaking changes our versioning properly reflects that.

[jenting]

Also any open questions and changes for #139 should be resolved before this goes to main. Are there other breaking changes under discussion? I can start a major version branch if we've got other stuff to roll together

Switched version to 3.0.0-alpha.1 so if we do merge and have additional breaking changes our versioning properly reflects that.

Sorry for the late review.

1. I like the idea to change the UX but I'd like it to have a backward-compatible which means we could support both

   helm install \
     ... \
     --set initContainers[0].name=velero-plugin-for-aws \
     --set initContainers[0].image=velero/velero-plugin-for-aws:v1.1.0 \
     --set initContainers[0].volumeMounts[0].mountPath=/target \
     --set initContainers[0].volumeMounts[0].name=plugins \
     --set initContainers[1].name=velero-plugin-for-csi \
     --set initContainers[1].image=velero/velero-plugin-for-csi:v0.1.1 \
     --set initContainers[1].volumeMounts[0].mountPath=/target \
     --set initContainers[1].volumeMounts[0].name=plugins
   

   and

   helm install \
     ... \
     --set plugins velero/velero-plugin-for-aws:v1.1.0,velero/velero-plugin-for-csi:v0.1.1
   

   Furthermore, if the user setup above at the same time, we'll remove the duplicate item.

2. I hope this did not remove the key configuration when setting up BSL/VSL, the same, for backward-compatibility.

3. I'm wondering about the purpose of changing the schedule template from {} to [].

But, thanks for bringing up the helm unit-test, I'm researching the helm unit test solution recently, I'll try it later~

[kav]

1. I like the idea to change the UX but I'd like it to have a backward-compatible which means we could support both

Yep initContainers are still available and function as before.

Furthermore, if the user setup above at the same time, we'll remove the duplicate item. 2. I hope this did not remove the key configuration when setting up BSL/VSL, the same, for backward-compatibility.

I did, but I suppose we can re-add fairly easily. If we are moving to a breaking change version do you see back compat as a large issue here? Seems like a Release note saying to remove the configuration key and place values at the root is a fairly minor amount of work for anyone migrating to the new version. On the other hand allowing new patterns without forcing them is probably friendlier to users.

3. I'm wondering about the purpose of changing the schedule template from {} to [].

I'm not sure if the pattern is driven by static typing in the various languages backing helm and kubernetes but it's uncommon, outside file name cases, to have a dynamic key as a name in values rather than an array of elements with name properties. As a new user of the Velero chart I tripped over this and expect other folks used to kubernetes and helm standards to do so as well. That said it was a change to match standard patterns if you folks prefer it as an object that's fine, this one is a bit subjective.

But, thanks for bringing up the helm unit-test, I'm researching the helm unit test solution recently, I'll try it later~

In general though we should discuss "back-compat" here. Part of the assumption in the changed UX is that there are some changes that will require breaking existing values structure. We can avoid that but it will complexify the chart logic as bit as we do. Perhaps allowing the new structures in a minor version and then enforcing them in a major version later, and removing the legacy structures then, is a better strategy if you're concerned about a smooth transition? That seems like a reasonable balanced solution. I'll update the PR.

[jenting]

1. I like the idea to change the UX but I'd like it to have a backward-compatible which means we could support both

Yep initContainers are still available and function as before.

Furthermore, if the user setup above at the same time, we'll remove the duplicate item. 2. I hope this did not remove the key configuration when setting up BSL/VSL, the same, for backward-compatibility.

I did, but I suppose we can re-add fairly easily. If we are moving to a breaking change version do you see back compat as a large issue here? Seems like a Release note saying to remove the configuration key and place values at the root is a fairly minor amount of work for anyone migrating to the new version. On the other hand allowing new patterns without forcing them is probably friendlier to users.

1. I'm wondering about the purpose of changing the schedule template from {} to [].

I'm not sure if the pattern is driven by static typing in the various languages backing helm and kubernetes but it's uncommon, outside file name cases, to have a dynamic key as a name in values rather than an array of elements with name properties. As a new user of the Velero chart I tripped over this and expect other folks used to kubernetes and helm standards to do so as well. That said it was a change to match standard patterns if you folks prefer it as an object that's fine, this one is a bit subjective.

But, thanks for bringing up the helm unit-test, I'm researching the helm unit test solution recently, I'll try it later~

In general though we should discuss "back-compat" here. Part of the assumption in the changed UX is that there are some changes that will require breaking existing values structure. We can avoid that but it will complexify the chart logic as bit as we do. Perhaps allowing the new structures in a minor version and then enforcing them in a major version later, and removing the legacy structures then, is a better strategy if you're concerned about a smooth transition? That seems like a reasonable balanced solution. I'll update the PR.

@carlisia @nrb do you have any other concerns about this PR?

[kav]

Added back compat and tests for configuration and resticEnabled keys. It's possible that multiple identical init containers will be created if a user has existing init container entries and the automatic provider based ones. I don't think that'll cause issues but some testing and eyes by someone other than myself would be appreciated. Next time a breaking changes release rolls we'll likely want to remove all the coalescing everywhere.

[kav]

Also added back compat and tests for schedules

[kav]

Ok, I think this is baked. Apologies for not setting it to draft it when I agreed to add backcompat

[airen29]

Thanks @kav for finding this out and for opening https://github.com/vmware-tanzu/helm-charts/issues/88. We had a problem in configuring secrets using sealedSecret and thanks to your Open PR we've managed to find what is the problem. Would be nice to actually have it updated already.

[kav]

y'all this branch is getting incredibly stale. I'm happy to bring it up to date but last time it was merge ready there were crickets from the repository team. Is there any interest in merging here or should I just go fork myself?

[esselius]

y'all this branch is getting incredibly stale. I'm happy to bring it up to date but last time it was merge ready there were crickets from the repository team. Is there any interest in merging here or should I just go fork myself?

Not a maintainer, but the changes in this PR seem great, but I'd recommend splitting them up into comprehensive chunks, as reviewing is a nightmare. Breakage might mean some backup system somewhere suddenly not working, which ain't great.

Love the changes otherwise!

[jenting]

I'd like to have separate PRs to addressing these issues, especially the PRs fulfill backward-compatible. For the non-backward-compatible change, we need to have a good way to notify the users after an upgrade, the existing configuration no longer valid and need to have some update before helm upgrade.

[kav]

This is fully back compatible and non-breaking for all changes at this point but for sure hear the concern about breaking backups; never a good look.

I can split out changes but they'll still likely need to be merged in sequence or the combinatorics of merge order and back compatible will get a bit nightmarish.