jmsperu commented on PR #12898: URL: https://github.com/apache/cloudstack/pull/12898#issuecomment-4523050536
Pushed 2150959 — addresses the last outstanding Copilot point (shared detail-key constants). Detail-map keys were duplicated as private literals in `NASBackupProvider` and `static final`s in `LibvirtTakeBackupCommandWrapper`. A typo/rename in one place silently broke the wire protocol. Moved them onto `TakeBackupCommand` itself (the class that carries the map between the two sides) so there's a single source of truth. Sweep of all Copilot threads on this PR: | # | Reviewer point | Status | |---|---|---| | 1 | encrypt_backup leaking mount on missing passphrase | fixed — `return 1` (earlier commit) | | 2 | verify_backup leaking mount on failure | fixed — `return 1` (earlier commit) | | 3 | qemu-img convert: cleanup not followed by exit/return | fixed — `return 1` after cleanup | | 4 | `> \"$logFile\"` truncating agent log | fixed — all sites use `>>` | | 5 | verify_backup not handling LUKS | fixed — `--object secret + --image-opts` | | 6 | encrypt step discarding compression | fixed — `-c` preserved via `$compress_flag` | | 7 | Provider: encryption-without-passphrase silent fallback | fixed — throws CloudRuntimeException | | 8 | Missing tests for new detail wirings | fixed — 5 new tests in NASBackupProviderTest | | 9 | Wrapper: same silent-fallback | fixed — returns BackupAnswer false | | 10 | Temp passphrase file leak / perms | fixed — 0600 + UTF-8 + catch+finally cleanup | | 11 | Hard-coded detail-key strings | **fixed this commit** — moved to TakeBackupCommand | | 12 | `@LogLevel(Off)` on details (secrets in logs) | fixed (earlier commit) | | 13 | setDetails null-safe | fixed (earlier commit) | Mergeable, no conflicts. Ready for re-review @abh1sar / @Copilot. -- 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]
