[erikbocks]

Description

Currently, the backup's path in the secondary storage is not displayed in the GUI, being necessary to query it directly from the database. Therefore, the backuppaths attribute, with the path of the backup of each VM's disk was added to the listBackups API response, and to the backup details in the GUI. Only Root Admin accounts are allowed to view the backup paths in the GUI and in the API response. Currently only supported by KBOSS provider.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Screenshots (if appropriate):

Backup paths in the GUI

How Has This Been Tested?

I configured KBOSS as the backup provider, and created a new backup of an existing VM. Then, I accessed the backup's details tab and validated that the new field was added successfully. I also validated that the backuppaths attribute was present at the listBackups API response.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[codecov]

Codecov Report

❌ Patch coverage is 24.52830% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 20.77%. Comparing base (a8200cf) to head (acf05ca).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ack/backup/dao/InternalBackupDataStoreDaoImpl.java	0.00%	17 Missing ⚠️
...k/api/response/helper/BackupVolumeNameAndPath.java	35.29%	11 Missing ⚠️
...apache/cloudstack/api/response/BackupResponse.java	50.00%	3 Missing ⚠️
...rg/apache/cloudstack/backup/BackupManagerImpl.java	57.14%	3 Missing ⚠️
...ck/api/command/admin/backup/FinalizeBackupCmd.java	0.00%	2 Missing ⚠️
...stack/api/command/admin/backup/StartBackupCmd.java	0.00%	2 Missing ⚠️
...dstack/api/command/user/backup/ListBackupsCmd.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #14255      +/-   ##
============================================
+ Coverage     20.73%   20.77%   +0.04%     
- Complexity    20781    20847      +66     
============================================
  Files          6049     6054       +5     
  Lines        547783   548224     +441     
  Branches      77899    73229    -4670     
============================================
+ Hits         113572   113888     +316     
- Misses       420893   420995     +102     
- Partials      13318    13341      +23     

Flag	Coverage Δ
uitests	3.86% <ø> (ø)
unittests	22.20% <24.52%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[erikbocks]

I forgot to change the test, so I will leave this one as draft for a bit before I can fix it.

[erikbocks]

@rajiv-jain-netapp, thanks for the reviews!

I made the following changes to this PR: Implemented null check for offerings, Changed how the backup path is stored in the response map and improved the test cases.

For the backup path map, thank you for pointing out about the same volume name replacement scenario. I changed the map to now use the volume UUID as key. Also, I created a new structure for the map value that stores the volume name and its image storage path as attributes.

Regarding the usage of storage data for the response, as the only value stored by internal_backup_view is the image store ID, obtained by fetching the backup details, I preferred not to include storage data because it would be necessary to include a new join on the image store table. Can you give your opinion on this?

For the tests, I extracted the BackupResponse related method to a separated class, so we could target the specific test cases for backup responses and reuse existing code for different scenarios. The following cases are addressed in this file:

• Default response (the test that already exists);
• KBOSS backup response for Admins;
• KBOSS backup response for normal users;
• KBOSS backup response with multiple equal named volumes;
• KBOSS backup response with null offering

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

6 open issues

• The PR is marked as non-breaking, but changing a public interface method signature is a… · New
• This view selects v.name/v.uuid but does not aggregate them or include them in GROUP BY. On… · New
• Object.values(dataResource[item]) will throw if dataResource.backuppaths is null/undefined… · New
• This class is used as a nested API response type but doesn’t include CloudStack-style response… · New
• createBackupResponse is called in a loop for listBackups, and this adds a DAO call per backup… · New
• Grammar nit: the description reads \"The backups paths\"; it should be \"The backup paths\" (or… · New

_{Review effort: Lite}

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

12 open issues

• dataResource[item] can be null for non-Root Admin users (per the intended behavior on the API… · New
• Object.values(dataResource[item]) will throw if dataResource.backuppaths is null/undefined…
• This view selects v.name/v.uuid but does not aggregate them or include them in GROUP BY. On…
• The PR is marked as non-breaking, but changing a public interface method signature is a…
• Changing the interface method signature is a source/binary breaking change for any out-of-tree… · New
• This view now selects v.name and v.uuid while grouping by only b.id, nbsr.volume_id.… · New
• When listBackups returns many results for a Root Admin on KBOSS, this adds a per-backup DAO call… · New
• createBackupResponse is called in a loop for listBackups, and this adds a DAO call per backup…
• This class is used as a nested API response type but doesn’t include CloudStack-style response…
• The description has a grammatical error: change “The backups paths” to “The backup paths”. · New
• Using the array index as the Vue key can cause incorrect re-rendering if the object entries… · New
• Grammar nit: the description reads \"The backups paths\"; it should be \"The backup paths\" (or…

_{Review effort: Lite}

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.