OADP-599: Auto create s3 restic secrets for volsync from BSLs #747
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
bot
added
do-not-merge/hold
openshift-ci
bot
requested review from
sseago
and removed request for
eemcmullan
|
/retest |
|
/test 4.9-operator-e2e-azure |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/retest-required |
There was a problem hiding this comment.
Changes look sane to me, Added some suggestions though. Thank you @savitharaghunathan
config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml
Outdated
Show resolved
Hide resolved
|
@shubham-pampattiwar Thanks for the elaborate review. I updated the func according to the last feedback. When you get a moment can you take a look at it please? |
|
/retest |
|
|
||
| for _, bsl := range backupStorageLocationList.Items { | ||
| if strings.Contains(bsl.Name, dpa.Name) { | ||
| _, err := r.createResticSecretsPerBSL(&dpa, bsl, dmresticsecretname, res_pass) |
There was a problem hiding this comment.
@savitharaghunathan Are we assuming that the given datamover secret will work for the BSL configured ? According to this current code, if I am not wrong, we are creating a dm secret per BSL and password provided in this secret will work for all the BSLs configured ?
For e.g:
- OADP has 2 BSLs - AWS and GCP. User supplied one dm secret,this PR will be creating restic secret using the dm secret for both the BSLs, right ? I think we should only create the restic secret for the dm one.
- OADP has 2 BSLs - Both AWS. Same issue.
Suggested Solution:
We add a BSLName and Provider field to Deature -> DataMover/ And we create the restic secret only for the BSL specified here. WDYT ?
There was a problem hiding this comment.
Please correct me if I am wrong here.
|
/test 4.8-ci-index |
bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml
Outdated
Show resolved
Hide resolved
|
/retest |
| if resticsecret == nil { | ||
| return false | ||
| } | ||
| for key, val := range resticsecret.Data { |
There was a problem hiding this comment.
We want to update this to check if the key exists at all, right now I get no error if the key isn't set
config/crd/bases/datamover.oadp.openshift.io_volumesnapshotbackups.yaml
Outdated
Show resolved
Hide resolved
|
@savitharaghunathan: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
shubham-pampattiwar
removed
the
do-not-merge/hold
|
/hold cancel |
No description provided.