JoaoJandre commented on code in PR #8782:
URL: https://github.com/apache/cloudstack/pull/8782#discussion_r1595405926
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java:
##########
@@ -239,22 +240,32 @@ public Long getAutoScaleVmGroupId() {
return autoScaleVmGroupId;
}
+ protected boolean isViewDetailsEmpty() {
+ return CollectionUtils.isEmpty(viewDetails);
+ }
+
public EnumSet<VMDetails> getDetails() throws
InvalidParameterValueException {
- EnumSet<VMDetails> dv;
- if (viewDetails == null || viewDetails.size() <= 0) {
- dv = EnumSet.of(VMDetails.all);
- } else {
- try {
- ArrayList<VMDetails> dc = new ArrayList<VMDetails>();
- for (String detail : viewDetails) {
- dc.add(VMDetails.valueOf(detail));
- }
- dv = EnumSet.copyOf(dc);
- } catch (IllegalArgumentException e) {
- throw new InvalidParameterValueException("The details
parameter contains a non permitted value. The allowed values are " +
EnumSet.allOf(VMDetails.class));
+ if (isViewDetailsEmpty()) {
+ if (_queryService.returnVmStatsOnVmList.value()) {
+ return EnumSet.of(VMDetails.all);
+ }
+
+ Set<VMDetails> allDetails = new
HashSet<>(Set.of(VMDetails.values()));
Review Comment:
There's a problem with this approach: When informing the `all` flag, the
stats will not be returned. When informing the `all` flag, every information
available should be returned.
The problem we have is assuming that when no flag is informed, the `all`
flag is presumed, when it should not be the default.
##########
api/src/main/java/org/apache/cloudstack/query/QueryService.java:
##########
@@ -125,6 +125,10 @@ public interface QueryService {
static final ConfigKey<Boolean> SharePublicTemplatesWithOtherDomains = new
ConfigKey<>("Advanced", Boolean.class,
"share.public.templates.with.other.domains", "true",
"If false, templates of this domain will not show up in the list
templates of other domains.", true, ConfigKey.Scope.Domain);
+ ConfigKey<Boolean> returnVmStatsOnVmList = new ConfigKey<>("Advanced",
Boolean.class, "return.vm.stats.on.vm.list", "true",
Review Comment:
I would really like to change the default behavior of this and some other
APIs. But we currently do not have a proper mechanism to do this. We are slowly
starting to discuss such mechanism in
https://github.com/apache/cloudstack/discussions/8970#discussion-6554582, and
more people discussing and giving ideas would be great.
In any case, since we are already creating a configuration for this, we
might as well leave the default behavior as is and let users set this
configuration if they feel the need to.
##########
plugins/metrics/src/main/java/org/apache/cloudstack/metrics/MetricsServiceImpl.java:
##########
@@ -617,7 +617,6 @@ public List<VmMetricsResponse>
listVmMetrics(List<UserVmResponse> vmResponses) {
}
metricsResponse.setHasAnnotation(vmResponse.hasAnnotation());
- metricsResponse.setIpAddress(vmResponse.getNics());
Review Comment:
The IP address will come from the UserVmResponse, so the metrics response
will be left as-is
##########
api/src/main/java/org/apache/cloudstack/api/response/UserVmResponse.java:
##########
@@ -269,6 +270,10 @@ public class UserVmResponse extends
BaseResponseWithTagInformation implements Co
@Param(description = "the hypervisor on which the template runs")
private String hypervisor;
+ @SerializedName(ApiConstants.IP_ADDRESS)
+ @Param(description = "the VM's primary IP address")
+ private String ipAddress;
Review Comment:
I don't see a reason to only return the primary IP on the metrics response,
I think it should be returned when listing the user VMs as well. Furthermore,
the metrics response extends the UserVmResponse, so when listing the metrics
the IP will still be returned.
--
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]
