simplify controller logic by adding status helper methods to MantleBackup and Restore#166
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies controller and test logic by replacing direct checks on status conditions with newly added helper methods (IsReady and IsSynced) in MantleBackup and MantleRestore. The changes remove redundant meta package usage from tests and controllers and update error checks to use the helper methods.
- Replaces meta.IsStatusConditionTrue calls with IsReady/IsSynced.
- Removes obsolete meta imports in several files.
- Updates tests and controllers to use the new helper methods.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/singlek8s/util.go | Updated backup readiness check using backup.IsReady() |
| test/e2e/singlek8s/restore_test.go | Changed restore condition assertion to use restore.IsReady() |
| test/e2e/multik8s/testutil/util.go | Replaced meta-based condition checks with IsReady()/IsSynced() and refined error handling |
| test/e2e/multik8s/misc_test.go | Updated multiple backup and restore condition assertions to use helper methods |
| test/e2e/multik8s/full_backup_test.go | Updated backup readiness check with IsReady() |
| internal/testutil/resources.go | Transitioned from meta calls to helper methods for backup conditions |
| internal/controller/replication.go | Simplified condition check in replication logic using IsReady() |
| internal/controller/mantlerestore_controller.go | Replaced restore condition check with IsReady() |
| internal/controller/mantlebackup_controller_test.go | Updated tests to use IsReady() and IsSynced() for backup conditions |
| internal/controller/mantlebackup_controller.go | Replaced condition checks with IsReady()/IsSynced() in replication and cleanup logic |
| api/v1/mantlerestore_types.go | Added IsReady() helper method for MantleRestore |
| api/v1/mantlebackup_types.go | Introduced IsReady() and IsSynced() helper methods for MantleBackup |
Comments suppressed due to low confidence (2)
test/e2e/multik8s/testutil/util.go:497
- [nitpick] Consider refining the error message in the surrounding context to include the actual status of the backup (or additional diagnostic information) to aid in troubleshooting when the backup is not ready.
if !mb.IsReady() {
api/v1/mantlebackup_types.go:90
- [nitpick] Add a documentation comment for IsReady() to clearly explain which condition it checks and the significance of the ReadyToUse status for API consumers.
func (m *MantleBackup) IsReady() bool {
|
@molpako |
Something is wrong with GitHub and I cannot remove the reviewers... |
satoru-takeuchi
left a comment
satoru-takeuchi
left a comment
There was a problem hiding this comment.
Just one minor nit.
| } | ||
|
|
||
| // isCreatedWhenMantleControllerWasSecondary returns true iff the MantleBackup | ||
| // isCreatedWhenMantleControllerWasSecondary returns true if the MantleBackup |
There was a problem hiding this comment.
'iff' is not a typo. It's intentional. It means "if and only if". Please keep it as is.
There was a problem hiding this comment.
"iff" will remain as written. Thanks for clarifying its meaning.
…ckup and Restore Signed-off-by: Kyori Sakao <sakaok@cybozu.co.jp>
molpako
force-pushed
the
refactor/move-helper-to-api-pkg
branch
from
13b947a to
164cf99
Compare
simplify controller logic by adding status helper methods to MantleBackup and Restore.