Add --plugins flag to velero install #1930
Conversation
Signed-off-by: Nolan Brubaker <[email protected]>
Signed-off-by: Nolan Brubaker <[email protected]>
| @@ -90,6 +91,7 @@ func (o *InstallOptions) BindFlags(flags *pflag.FlagSet) { | |||
| flags.BoolVar(&o.UseRestic, "use-restic", o.UseRestic, "create restic deployment. Optional.") | |||
| flags.BoolVar(&o.Wait, "wait", o.Wait, "wait for Velero deployment to be ready. Optional.") | |||
| flags.DurationVar(&o.DefaultResticMaintenanceFrequency, "default-restic-prune-frequency", o.DefaultResticMaintenanceFrequency, "how often 'restic prune' is run for restic repositories by default. Optional.") | |||
| flags.Var(&o.Plugins, "plugins", "Plugin container images to install into the Velero Deployment. Optional.") | |||
There was a problem hiding this comment.
Isn't this supposed to be required?
There was a problem hiding this comment.
Or should I change it to required on my PR that extracts the cloudproviders?
There was a problem hiding this comment.
I can change it now if you'd like; I actually need to add a change log anyway.
It's a bit awkward either way at the moment - if it's required now, we basically have to add a dummy plugin image to get velero install to work on master, as the cloud provider plugins aren't present yet.
If it's optional once the plugins are out of tree, then velero install could install a deployment without any object storage plugins present, which would mean it's nonfunctional.
There was a problem hiding this comment.
Let me change it to required as part of the PR that extracts the providers then. That will only go in once we have the plugin repos and their images.
|
Should any documentation be added now? |
|
Or, would the documentation for this be this issue: #1884? |
Signed-off-by: Nolan Brubaker <[email protected]>
Signed-off-by: Nolan Brubaker <[email protected]>
|
I opened #1884 as an alternative to this. I think the documentation that makes sense for this PR is to document that the plugin flag exists and will install any specified container image into the deployment. Specific cloud provider instructions can be done in their respective PR(s). |
|
That makes sense to me re: documentation! Will review the updated code shortly. |
Signed-off-by: Nolan Brubaker <[email protected]>
| @@ -0,0 +1 @@ | |||
| Add a new required --plugins flag for velero install command. --plugins takes a list of container images to add as initcontainers. | |||
There was a problem hiding this comment.
Technically it's optional now, but will be required by the time we release - I'm fine leaving it as-is given that it'll change, but wasn't sure if it was intentional or not.
There was a problem hiding this comment.
I'll change it to required when I merge the cloud provider extraction PR. I added a todo for that.
Fixes #1740