jmsperu commented on code in PR #13877:
URL: https://github.com/apache/cloudstack/pull/13877#discussion_r3844116591
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -974,6 +974,10 @@ public Pair<List<Backup>, Integer> listBackups(final
ListBackupsCmd cmd) {
sb.and("name", sb.entity().getName(), SearchCriteria.Op.EQ);
sb.and("zoneId", sb.entity().getZoneId(), SearchCriteria.Op.EQ);
sb.and("backupOfferingId", sb.entity().getBackupOfferingId(),
SearchCriteria.Op.EQ);
+ // Tombstoned chain backups (Status.Hidden) are never shown to users;
they exist only so the
+ // incremental chain GC can sweep them once their last descendant is
deleted.
+ sb.and("statusNeq", sb.entity().getStatus(), SearchCriteria.Op.NEQ);
+ sb.and("backupStatus", sb.entity().getStatus(), SearchCriteria.Op.EQ);
Review Comment:
Checked this against 4.22. The `backupStatus` condition came across from
main, where #13254 added the status filter to listBackups, but 4.22 has neither
`ListBackupsCmd.getBackupStatus()` nor the
`sc.setParametersIfNotNull("backupStatus", ...)` call, so here it is an
`sb.and()` whose parameter is never set. It is harmless rather than fatal:
`SearchCriteria.getWhereClause()` skips any condition whose parameters were
never set, so listing still works. It is dead code in this branch though, so
I'd drop line 980 and keep only the `statusNeq` clause.
##########
test/integration/smoke/test_backup_recovery_nas.py:
##########
@@ -56,13 +56,17 @@ def setUpClass(cls):
# Check backup configuration values, set them to enable the nas
provider
backup_enabled_cfg = Configurations.list(cls.api_client,
name='backup.framework.enabled')
backup_provider_cfg = Configurations.list(cls.api_client,
name='backup.framework.provider.plugin')
+ incremental_backup_enabled_cfg = Configurations.list(cls.api_client,
name='nas.backup.incremental.enabled')
cls.backup_enabled = backup_enabled_cfg[0].value
cls.backup_provider = backup_provider_cfg[0].value
+ cls.incremental_backup_enabled =
incremental_backup_enabled_cfg[0].value
Review Comment:
This mirrors the merged test on main line for line, so it isn't something
the backport introduced. Worth fixing in both places; I'll put up a small
follow-up on main that passes `zoneid=cls.zone.id` on all three reads so the
two branches stay in sync.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
