Copilot commented on code in PR #13288:
URL: https://github.com/apache/cloudstack/pull/13288#discussion_r3324560147
##########
server/src/main/java/com/cloud/api/query/dao/DomainJoinDaoImpl.java:
##########
@@ -151,6 +151,14 @@ public void setResourceLimits(DomainJoinVO domain, boolean
fullView, ResourceLim
response.setSnapshotTotal(snapshotTotal);
response.setSnapshotAvailable(snapshotAvail);
+ Long vmSnapshotLimit =
ApiDBUtils.findCorrectResourceLimitForDomain(domain.getVmSnapshotLimit(),
ResourceType.vm_snapshot, domain.getId());
Review Comment:
In the domain resource limits, VM snapshot limits are currently resolved
using the `findCorrectResourceLimitForDomain(Long, ResourceType, long)`
overload, which falls back to the global default when the view column is null.
This differs from the VM limit logic (which uses the overload that handles
ROOT-domain/unlimited and resolves inherited limits correctly). Use the same
overload as `vmLimit` to ensure ROOT domain shows unlimited and missing limits
resolve via the domain hierarchy as expected.
##########
server/src/main/java/com/cloud/api/query/dao/AccountJoinDaoImpl.java:
##########
@@ -172,6 +172,14 @@ public void setResourceLimits(AccountJoinVO account,
boolean fullView, ResourceL
response.setSnapshotTotal(snapshotTotal);
response.setSnapshotAvailable(snapshotAvail);
+ Long vmSnapshotLimit =
ApiDBUtils.findCorrectResourceLimit(account.getVmSnapshotLimit(),
account.getId(), ResourceType.vm_snapshot);
+ String vmSnapshotLimitDisplay = (fullView || vmSnapshotLimit == -1) ?
Resource.UNLIMITED : String.valueOf(vmSnapshotLimit);
+ Long vmSnapshotTotal = (account.getVmSnapshotTotal() == null) ? 0 :
account.getVmSnapshotTotal();
+ String snapshotAvailable = (fullView || vmSnapshotLimit == -1) ?
Resource.UNLIMITED : String.valueOf(vmSnapshotLimit - vmSnapshotTotal);
+ response.setVmSnapshotLimit(vmSnapshotLimitDisplay);
+ response.setVmSnapshotTotal(vmSnapshotTotal);
+ response.setVmSnapshotAvailable(snapshotAvailable);
Review Comment:
The local variable used to hold the VM snapshot available count is named
`snapshotAvailable`, which is misleading next to the existing snapshot
variables and makes the code harder to follow. Rename it to
`vmSnapshotAvailable` to match the values being computed and set on the
response.
##########
api/src/main/java/org/apache/cloudstack/api/command/user/resource/ListResourceLimitsCmd.java:
##########
@@ -42,7 +42,7 @@ public class ListResourceLimitsCmd extends
BaseListProjectAndAccountResourcesCmd
@Parameter(name = ApiConstants.ID, type = CommandType.LONG, description =
"Lists resource limits by ID.")
private Long id;
- @Parameter(name = ApiConstants.RESOURCE_TYPE, type = CommandType.INTEGER,
description = "Type of resource. Values are 0, 1, 2, 3, 4, 6, 7, 8, 9, 10 and
11. "
+ @Parameter(name = ApiConstants.RESOURCE_TYPE, type = CommandType.INTEGER,
description = "Type of resource. Values are 0, 1, 2, 3, 4, 6, 7, 8, 9, 10, 11
and 12. "
Review Comment:
The resourceType parameter description says valid values are "0, 1, 2, 3, 4,
6, ..." but the same description then lists "5 - Project" as a valid option.
Include 5 in the initial list to keep the API documentation consistent.
##########
api/src/main/java/org/apache/cloudstack/api/response/ProjectResponse.java:
##########
@@ -188,6 +188,18 @@ public class ProjectResponse extends BaseResponse
implements ResourceLimitAndCou
@Param(description = "The total number of Snapshots available for this
project", since = "4.2.0")
private String snapshotAvailable;
+ @SerializedName(ApiConstants.VM_SNAPSHOT_LIMIT)
+ @Param(description = "the number of VM snapshots that can be stored by
this account")
+ private String vmSnapshotLimit;
+
+ @SerializedName(ApiConstants.VM_SNAPSHOT_TOTAL)
+ @Param(description = "the number of VM snapshots stored by this account")
+ private Long vmSnapshotTotal;
+
+ @SerializedName(ApiConstants.VM_SNAPSHOT_AVAILABLE)
+ @Param(description = "the number of VM snapshots available for this
account")
+ private String vmSnapshotAvailable;
Review Comment:
These `@Param` descriptions reference "this account", but the fields are
part of ProjectResponse and represent limits/usage for the project. Update the
text to avoid confusing API consumers.
##########
api/src/main/java/org/apache/cloudstack/api/response/DomainResponse.java:
##########
@@ -105,6 +105,18 @@ public class DomainResponse extends
BaseResponseWithAnnotations implements Resou
@SerializedName("snapshotavailable") @Param(description = "The total
number of Snapshots available for this domain")
private String snapshotAvailable;
+ @SerializedName(ApiConstants.VM_SNAPSHOT_LIMIT)
+ @Param(description = "the number of VM snapshots that can be stored by
this account")
+ private String vmSnapshotLimit;
+
+ @SerializedName(ApiConstants.VM_SNAPSHOT_TOTAL)
+ @Param(description = "the number of VM snapshots stored by this account")
+ private Long vmSnapshotTotal;
+
+ @SerializedName(ApiConstants.VM_SNAPSHOT_AVAILABLE)
+ @Param(description = "the number of VM snapshots available for this
account")
+ private String vmSnapshotAvailable;
Review Comment:
These `@Param` descriptions reference "this account", but the fields are
part of DomainResponse and represent limits/usage for the domain. Update the
text to avoid confusing API consumers.
##########
api/src/main/java/org/apache/cloudstack/api/command/user/resource/UpdateResourceCountCmd.java:
##########
@@ -60,7 +60,7 @@ public class UpdateResourceCountCmd extends BaseCmd {
@Parameter(name = ApiConstants.RESOURCE_TYPE,
type = CommandType.INTEGER,
- description = "Type of resource to update. If specifies valid
values are 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 and 11. If not specified will
update all resource counts"
+ description = "Type of resource to update. If specifies valid
values are 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11 and 12. If not specified will
update all resource counts"
Review Comment:
The parameter description has a grammatical issue ("If specifies") and the
concatenation currently misses punctuation/spacing between "...resource counts"
and the first enumerated item, which makes the generated API docs harder to
read. Adjust the first sentence to be grammatically correct and end it with a
period and space.
--
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]
