-
Notifications
You must be signed in to change notification settings - Fork 48
Ability to use to CSI plugin for Velero. Supports AWS EBS CSI driver. #1285
base: master
Are you sure you want to change the base?
Conversation
@ipochi component name is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits, overall LGTM
pkg/components/velero/csi/csi.go
Outdated
func (c *Configuration) Validate() hcl.Diagnostics { | ||
return c.validate() | ||
} | ||
|
||
// validate validates component configuration | ||
func (c *Configuration) validate() hcl.Diagnostics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to have those 2 function separate?
} | ||
|
||
// validate validates BackupStorageLocation struct fields. | ||
func (b *BackupStorageLocation) validate() hcl.Diagnostics { // nolint:dupl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could deduplicate the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean:
$ git grep 'BackupStorageLocation) validate'
pkg/components/velero/csi/drivers/aws/aws.go:func (b *BackupStorageLocation) validate() hcl.Diagnostics {
pkg/components/velero/openebs/openebs.go:func (b *BackupStorageLocation) validate(defaultProvider string) hcl.Diagnostics { // nolint:dupl
This code is very similar, we can perhaps put it in separate package and import to avoid duplication.
|
||
// Values returns the plugin specific values for Velero Helm chart. | ||
func (c *Configuration) Values() (string, error) { | ||
t := template.Must(template.New("values").Parse(chartValuesTmpl)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use template.Must
we need to have unit tests to validate the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/components/velero/component_test.go
has the tests for each provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh.. Can we have a tests locally to the package? Otherwise changing code in one package breaks tests in other, this is not nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added unit tests.
4528838
to
7e996aa
Compare
7e996aa
to
7c61da3
Compare
This adds the ability to create backup and restore using CSI plugin when Lokomotive is installed on AWS and `aws_ebs_csi_driver` componnent is used to dynamically provision volumes. Signed-off-by: Imran Pochi <[email protected]>
Update configuration reference documentation to reflect the new CSI plugin. Signed-off-by: Imran Pochi <[email protected]>
0145eea
to
db02ca3
Compare
@ipochi what's the progress on this? Is this being worked on actively? |
@surajssd yes. This would not be part of the 0.6.0 release. I didn't get much time to address the review comments as I am focussing on baremetal platform missing features. |
This adds the ability to create backup and restore with Velero using CSI plugin when Lokomotive is installed on AWS and
aws-ebs-csi-driver
component is used to dynamically provision volumes