delete the existing backuprepositories prior to a new test #1521
Conversation
openshift-ci
bot
added
the
approved
|
Is this a VIRT only problem? |
weshayutin
force-pushed
the
remove_backuprepo_cirros_test
branch
from
49691f4
to
b9a777e
Compare
It's exposed with Virt simply because we have so many tests using the same namespace. It should also be a problem with DM tests that use the namespace. There is nothing bad about removing the backuprepo object in between tests. |
|
/retest |
1 similar comment
|
/retest |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
wouldn't this be too much cleanup and not representative of a real world usage? |
We can and should have multiple b/r on the same namespace and we can write those tests. None of the current tests are designed for that atm imho. Each test is meant to be an initial backup. I am rewriting this to kill all the backuprepository's found. |
weshayutin
force-pushed
the
remove_backuprepo_cirros_test
branch
from
b9a777e
to
a762df0
Compare
|
We actually run multiple backups on the same BSL and so currently backup repositories is reused. I am ok with this change, we can explicitly test in future tests |
|
/retest |
weshayutin
changed the title
weshayutin
force-pushed
the
remove_backuprepo_cirros_test
branch
from
a762df0
to
181d738
Compare
|
@mateusoliveira43 look ok to you now? Thanks for the reviews and suggestions! |
|
|
||
| backupRepos, err := GetBackupRepositoryList(c, namespace) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get BackupRepository list: %v", err) |
There was a problem hiding this comment.
would check if error was not found error, and it is, return nil (there are no BackupRepositories in this namespace, nothing to delete), also keeping generic error handling as it is now
There was a problem hiding this comment.
I think this will break tests, first run will always fail, because there is no prior backupRepo, and and error will be returned
need to check if the error is not found error here for cases where there is no prior backupRepo,no?
There was a problem hiding this comment.
I had the same worry as you are expressing.
on line 150 I have
backupRepositoryList := &velero.BackupRepositoryList{
Items: []velero.BackupRepository{},
which should initialize the list w/ 0 items.
Unless I'm misunderstanding something, GetBackupRepositoryList should at minimum return an empty list or a populated list.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…oreAll Signed-off-by: Wesley Hayutin <[email protected]>
Signed-off-by: Wesley Hayutin <[email protected]>
Signed-off-by: Wesley Hayutin <[email protected]>
Signed-off-by: Wesley Hayutin <[email protected]>
weshayutin
force-pushed
the
remove_backuprepo_cirros_test
branch
from
181d738
to
58dbb8c
Compare
Signed-off-by: Wesley Hayutin <[email protected]>
|
@weshayutin: 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-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shubham-pampattiwar, weshayutin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Why the changes were made
delete the backuprepository for cirros-test namespace if found.
The e2e tests use a new bsl for every test run but the backuprepository can be stale and prevent backups from passing.
We need to delete backuprepository as well.
How to test the changes made
make test-e2e with virt settings