Prioritize location provider over global provider

[yurinnick]

Closes #95.

Prioritize configuration.(backupStorageLocation|volumeSnapshotLocation).config over configuration.provider

Signed-off-by: Nikolai Iurin yurinnick93@gmail.com

[carlisia]

In the line below, if there's a value for provider, it will make that the default value for the backupStorageLocation.provider anyway. Unless I'm reading this wrong. If it's right, would this be necessary?

https://github.com/vmware-tanzu/helm-charts/blob/master/charts/velero/templates/_helpers.tpl#L92

[yurinnick]

Currently, if configuration.provider is set (and it has to be to enable deployment) it overrides configuration.backupStorageLocation.provider. Why? Because the first value is actually the default and the second one if given: doc. Not really intuitive, but it is how it is.

[carlisia]

I appreciate the explanation! My understanding was the exact opposite, so, glad to have this corrected. Thanks for the link to the relevant docs, I'll revisit that too.

[austinbv]

I just submitted this same thing as a PR and closed it b/c this is here #98

@carlisia helm documentation is default DEFAULT_VALUE GIVEN_VALUE see https://v2.helm.sh/docs/chart_template_guide/#using-the-default-function

This threw me through a loop too but if this PR is merged the chart will default to configuration.provider if there is no more specific provider defined in configuration.backupStorageLocation or configuration.volumeSnapshotLocation.

This PR is huge as it lets us define separate providers for both the volumeSnapshotLocation and backupStorageLocation and define a configuration.provider which is required in the deployment here https://github.com/vmware-tanzu/helm-charts/blob/master/charts/velero/templates/deployment.yaml#L1-L2

[carlisia]

Ok, makes sense. @yurinnick let's bump the chart version and get it merged.

[carlisia]

Thank you for this! 👍

[skriss]

thanks for this @yurinnick, this change makes sense to me.

It'd be great if you could also update the comments in values.yaml to describe the new precedence here.

[skriss]

should this be {{ default .provider .backupStorageLocation.name }}? .backupStorageLocation.name is what I see documented in values.yaml and the docs. However, I also see on L90 that we're using {{- define "velero.backupStorageLocation.provider" -}}, and in the backupstoragelocation template we reference backupStorageLocation.provider (https://github.com/vmware-tanzu/helm-charts/blob/master/charts/velero/templates/backupstoragelocation.yaml#L12).

Not entirely sure but it seems like there's an inconsistency somewhere.

[austinbv]

hey @skriss I see name and provider being a bit decoupled - name is used for the name of a resource while the provider is used by the plugin for config.

To use name you actually introduce complexity - for example when using digital ocean you would set your provider as digitalocean.com/velero which is not a valid kubernetes resource name because of the /

Another use case would be more than one AWS providers with different names

[skriss]

I totally agree they should be separate things. I'm just trying to ensure that (a) we're not making a backwards-incompatible change if we were previously using the "name" value to fill in the provider and now we're no longer doing so; and (b) that the docs are accurate.

I can't really tell at a glance if we're making a backwards-incompatible change here, but it seems like probably not?

If not, then let's please just document that there is now a backupStorageLocation.provider that is separate from backupStorageLocation.name.

[skriss]

same question here

[austinbv]

see above

[austinbv]

@skriss I didn't have permission to modify this but if you merge #100 along with this change I think the documentation will be included.