DaanHoogland commented on code in PR #5797:
URL: https://github.com/apache/cloudstack/pull/5797#discussion_r1062344189
##########
api/src/main/java/com/cloud/server/ManagementService.java:
##########
@@ -102,6 +104,13 @@ public interface ManagementService {
*/
Pair<List<? extends Configuration>, Integer>
searchForConfigurations(ListCfgsByCmd c);
+ /**
+ * returns the the configuration groups
+ *
+ * @return list of configuration groups
+ */
+ Pair<List<? extends ConfigurationGroup>, Integer>
listConfigurationGroups(ListCfgGroupsByCmd cmd);
Review Comment:
Any reason this would not suffice?
```suggestion
Pair<List<ConfigurationGroup>, Integer>
listConfigurationGroups(ListCfgGroupsByCmd cmd);
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/config/ListCfgGroupsByCmd.java:
##########
@@ -0,0 +1,79 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.api.command.admin.config;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.BaseListCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.ConfigurationGroupResponse;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.config.ConfigurationGroup;
+import org.apache.log4j.Logger;
+
+import com.cloud.utils.Pair;
+
+@APICommand(name = ListCfgGroupsByCmd.APINAME, description = "Lists all
configuration groups (primarily used for UI).", responseObject =
ConfigurationGroupResponse.class,
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
since = "4.18.0")
+public class ListCfgGroupsByCmd extends BaseListCmd {
+ public static final Logger s_logger =
Logger.getLogger(ListCfgGroupsByCmd.class.getName());
+
+ public static final String APINAME = "listConfigurationGroups";
+
+ // ///////////////////////////////////////////////////
+ // ////////////// API parameters /////////////////////
+ // ///////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.GROUP, type = CommandType.STRING,
description = "lists configuration group by group name")
+ private String groupName;
+
+ // ///////////////////////////////////////////////////
+ // ///////////////// Accessors ///////////////////////
+ // ///////////////////////////////////////////////////
+
+ public String getGroupName() {
+ return groupName;
+ }
+
+ // ///////////////////////////////////////////////////
+ // ///////////// API Implementation///////////////////
+ // ///////////////////////////////////////////////////
+
+ @Override
+ public String getCommandName() {
+ return APINAME.toLowerCase() + BaseCmd.RESPONSE_SUFFIX;
+ }
+
+ @Override
+ public void execute() {
+ Pair<List<? extends ConfigurationGroup>, Integer> result =
_mgr.listConfigurationGroups(this);
Review Comment:
```suggestion
Pair<List<ConfigurationGroup>, Integer> result =
_mgr.listConfigurationGroups(this);
```
?
##########
framework/spring/lifecycle/src/main/java/org/apache/cloudstack/spring/lifecycle/registry/ExtensionRegistry.java:
##########
@@ -136,14 +136,17 @@ public ConfigKey<?>[] getConfigKeys() {
List<ConfigKey<String>> result = new ArrayList<ConfigKey<String>>();
if (orderConfigKey != null && orderConfigKeyObj == null) {
- orderConfigKeyObj = new ConfigKey<String>("Advanced",
String.class, orderConfigKey, orderConfigDefault, "The order of precedence for
the extensions", false);
+ orderConfigKeyObj = new ConfigKey<String>(String.class,
orderConfigKey, "Advanced", orderConfigDefault, "The order of precedence for
the extensions", false, ConfigKey.Scope.Global, null, null, null, null, null,
ConfigKey.Kind.Order, orderConfigDefault);
Review Comment:
```suggestion
orderConfigKeyObj = new ConfigKey<>(String.class,
orderConfigKey, "Advanced", orderConfigDefault, "The order of precedence for
the extensions", false, ConfigKey.Scope.Global, null, null, null, null, null,
ConfigKey.Kind.Order, orderConfigDefault);
```
##########
framework/spring/lifecycle/src/main/java/org/apache/cloudstack/spring/lifecycle/registry/ExtensionRegistry.java:
##########
@@ -136,14 +136,17 @@ public ConfigKey<?>[] getConfigKeys() {
List<ConfigKey<String>> result = new ArrayList<ConfigKey<String>>();
if (orderConfigKey != null && orderConfigKeyObj == null) {
- orderConfigKeyObj = new ConfigKey<String>("Advanced",
String.class, orderConfigKey, orderConfigDefault, "The order of precedence for
the extensions", false);
+ orderConfigKeyObj = new ConfigKey<String>(String.class,
orderConfigKey, "Advanced", orderConfigDefault, "The order of precedence for
the extensions", false, ConfigKey.Scope.Global, null, null, null, null, null,
ConfigKey.Kind.Order, orderConfigDefault);
}
+ // orderConfigKeyObj = new ConfigKey<String>("Advanced", String.class,
orderConfigKey, orderConfigDefault, "The order of precedence for the
extensions", false, Scope.Global, null, null, null, null, null, null, null);
+
+
if (orderConfigKeyObj != null)
result.add(orderConfigKeyObj);
if (excludeKey != null && excludeKeyObj == null) {
- excludeKeyObj = new ConfigKey<String>("Advanced", String.class,
excludeKey, excludeDefault, "Extensions to exclude from being registered",
false);
+ excludeKeyObj = new ConfigKey<String>(String.class, excludeKey,
"Advanced", excludeDefault, "Extensions to exclude from being registered",
false, ConfigKey.Scope.Global, null, null, null, null, null,
ConfigKey.Kind.CSV, null);
Review Comment:
```suggestion
excludeKeyObj = new ConfigKey<>(String.class, excludeKey,
"Advanced", excludeDefault, "Extensions to exclude from being registered",
false, ConfigKey.Scope.Global, null, null, null, null, null,
ConfigKey.Kind.CSV, null);
```
##########
engine/schema/src/main/resources/META-INF/db/schema-41720to41800.sql:
##########
@@ -993,3 +993,246 @@ BEGIN
DECLARE CONTINUE HANDLER FOR 1061 BEGIN END; SET @ddl = CONCAT('ALTER
TABLE ', in_table_name); SET @ddl = CONCAT(@ddl, ' ', ' ADD KEY ') ; SET @ddl =
CONCAT(@ddl, ' ', in_index_name); SET @ddl = CONCAT(@ddl, ' ',
in_key_definition); PREPARE stmt FROM @ddl; EXECUTE stmt; DEALLOCATE PREPARE
stmt; END;
CALL `cloud`.`IDEMPOTENT_ADD_KEY`('i_user_ip_address_state','user_ip_address',
'(state)');
+--
+-- Update Configuration Groups and Subgroups
+--
Review Comment:
none of the add columns below are idem potent
##########
api/src/main/java/org/apache/cloudstack/ca/CAManager.java:
##########
@@ -46,10 +46,10 @@ public interface CAManager extends CAService, Configurable,
PluggableService {
"2048",
"The key size to be used for random
certificate keypair generation.", true);
- ConfigKey<String> CertSignatureAlgorithm = new ConfigKey<>("Advanced",
String.class,
- "ca.framework.cert.signature.algorithm",
+ ConfigKey<String> CertSignatureAlgorithm = new
ConfigKey<String>(String.class,
Review Comment:
```suggestion
ConfigKey<String> CertSignatureAlgorithm = new ConfigKey<>(String.class,
```
##########
api/src/main/java/org/apache/cloudstack/query/QueryService.java:
##########
@@ -94,13 +94,13 @@ public interface QueryService {
ConfigKey<Boolean> AllowUserViewDestroyedVM = new ConfigKey<>("Advanced",
Boolean.class, "allow.user.view.destroyed.vm", "false",
"Determines whether users can view their destroyed or expunging vm
", true, ConfigKey.Scope.Account);
- static final ConfigKey<String> UserVMDeniedDetails = new
ConfigKey<String>("Advanced", String.class,
- "user.vm.denied.details", "rootdisksize, cpuOvercommitRatio,
memoryOvercommitRatio, Message.ReservedCapacityFreed.Flag",
- "Determines whether users can view certain VM settings. When set
to empty, default value used is: rootdisksize, cpuOvercommitRatio,
memoryOvercommitRatio, Message.ReservedCapacityFreed.Flag.", true);
+ static final ConfigKey<String> UserVMDeniedDetails = new
ConfigKey<String>(String.class,
Review Comment:
```suggestion
static final ConfigKey<String> UserVMDeniedDetails = new
ConfigKey<>(String.class,
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/config/ListCfgsByCmd.java:
##########
@@ -145,35 +179,69 @@ public Long getPageSizeVal() {
@Override
public void execute() {
- Pair<List<? extends Configuration>, Integer> result =
_mgr.searchForConfigurations(this);
- ListResponse<ConfigurationResponse> response = new
ListResponse<ConfigurationResponse>();
- List<ConfigurationResponse> configResponses = new
ArrayList<ConfigurationResponse>();
- for (Configuration cfg : result.first()) {
- ConfigurationResponse cfgResponse =
_responseGenerator.createConfigurationResponse(cfg);
- cfgResponse.setObjectName("configuration");
- if (getZoneId() != null) {
- cfgResponse.setScope("zone");
- }
- if (getClusterId() != null) {
- cfgResponse.setScope("cluster");
- }
- if (getStoragepoolId() != null) {
- cfgResponse.setScope("storagepool");
+ validateParameters();
+ try {
+ Pair<List<? extends Configuration>, Integer> result =
_mgr.searchForConfigurations(this);
+ ListResponse<ConfigurationResponse> response = new
ListResponse<ConfigurationResponse>();
+ List<ConfigurationResponse> configResponses = new
ArrayList<ConfigurationResponse>();
Review Comment:
```suggestion
ListResponse<ConfigurationResponse> response = new
ListResponse<>();
List<ConfigurationResponse> configResponses = new ArrayList<>();
```
##########
api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java:
##########
@@ -20,15 +20,15 @@
import org.apache.cloudstack.framework.config.Configurable;
public class ApiServiceConfiguration implements Configurable {
- public static final ConfigKey<String> ManagementServerAddresses = new
ConfigKey<String>("Advanced", String.class, "host", "localhost", "The ip
address of management server. This can also accept comma separated addresses.",
true);
+ public static final ConfigKey<String> ManagementServerAddresses = new
ConfigKey<String>(String.class, "host", "Advanced", "localhost", "The ip
address of management server. This can also accept comma separated addresses.",
true, ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.CSV,
null);
public static final ConfigKey<String> ApiServletPath = new
ConfigKey<String>("Advanced", String.class, "endpoint.url",
"http://localhost:8080/client/api";,
"API end point. Can be used by CS components/services deployed
remotely, for sending CS API requests", true);
public static final ConfigKey<Long> DefaultUIPageSize = new
ConfigKey<Long>("Advanced", Long.class, "default.ui.page.size", "20",
"The default pagesize to be used by UI and other clients when
making list* API calls", true, ConfigKey.Scope.Global);
public static final ConfigKey<Boolean> ApiSourceCidrChecksEnabled = new
ConfigKey<>("Advanced", Boolean.class, "api.source.cidr.checks.enabled",
"true", "Are the source checks on API calls enabled (true) or not
(false)? See api.allowed.source.cidr.list", true, ConfigKey.Scope.Global);
- public static final ConfigKey<String> ApiAllowedSourceCidrList = new
ConfigKey<String>("Advanced", String.class, "api.allowed.source.cidr.list",
- "0.0.0.0/0,::/0", "Comma separated list of IPv4/IPv6 CIDRs from
which API calls can be performed. Can be set on Global and Account levels.",
true, ConfigKey.Scope.Account);
+ public static final ConfigKey<String> ApiAllowedSourceCidrList = new
ConfigKey<String>(String.class, "api.allowed.source.cidr.list", "Advanced",
Review Comment:
```suggestion
public static final ConfigKey<String> ApiAllowedSourceCidrList = new
ConfigKey<>(String.class, "api.allowed.source.cidr.list", "Advanced",
```
##########
api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java:
##########
@@ -20,15 +20,15 @@
import org.apache.cloudstack.framework.config.Configurable;
public class ApiServiceConfiguration implements Configurable {
- public static final ConfigKey<String> ManagementServerAddresses = new
ConfigKey<String>("Advanced", String.class, "host", "localhost", "The ip
address of management server. This can also accept comma separated addresses.",
true);
+ public static final ConfigKey<String> ManagementServerAddresses = new
ConfigKey<String>(String.class, "host", "Advanced", "localhost", "The ip
address of management server. This can also accept comma separated addresses.",
true, ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.CSV,
null);
Review Comment:
```suggestion
public static final ConfigKey<String> ManagementServerAddresses = new
ConfigKey<>(String.class, "host", "Advanced", "localhost", "The ip address of
management server. This can also accept comma separated addresses.", true,
ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.CSV, null);
```
##########
api/src/main/java/org/apache/cloudstack/query/QueryService.java:
##########
@@ -94,13 +94,13 @@ public interface QueryService {
ConfigKey<Boolean> AllowUserViewDestroyedVM = new ConfigKey<>("Advanced",
Boolean.class, "allow.user.view.destroyed.vm", "false",
"Determines whether users can view their destroyed or expunging vm
", true, ConfigKey.Scope.Account);
- static final ConfigKey<String> UserVMDeniedDetails = new
ConfigKey<String>("Advanced", String.class,
- "user.vm.denied.details", "rootdisksize, cpuOvercommitRatio,
memoryOvercommitRatio, Message.ReservedCapacityFreed.Flag",
- "Determines whether users can view certain VM settings. When set
to empty, default value used is: rootdisksize, cpuOvercommitRatio,
memoryOvercommitRatio, Message.ReservedCapacityFreed.Flag.", true);
+ static final ConfigKey<String> UserVMDeniedDetails = new
ConfigKey<String>(String.class,
+ "user.vm.denied.details", "Advanced", "rootdisksize, cpuOvercommitRatio,
memoryOvercommitRatio, Message.ReservedCapacityFreed.Flag",
+ "Determines whether users can view certain VM settings. When set
to empty, default value used is: rootdisksize, cpuOvercommitRatio,
memoryOvercommitRatio, Message.ReservedCapacityFreed.Flag.", true,
ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.CSV, null);
- static final ConfigKey<String> UserVMReadOnlyDetails = new
ConfigKey<String>("Advanced", String.class,
- "user.vm.readonly.details", "dataDiskController,
rootDiskController",
- "List of read-only VM settings/details as comma separated string",
true);
+ static final ConfigKey<String> UserVMReadOnlyDetails = new
ConfigKey<String>(String.class,
Review Comment:
```suggestion
static final ConfigKey<String> UserVMReadOnlyDetails = new
ConfigKey<>(String.class,
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/config/ListCfgGroupsByCmd.java:
##########
@@ -0,0 +1,79 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.api.command.admin.config;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.BaseListCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.ConfigurationGroupResponse;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.config.ConfigurationGroup;
+import org.apache.log4j.Logger;
+
+import com.cloud.utils.Pair;
+
+@APICommand(name = ListCfgGroupsByCmd.APINAME, description = "Lists all
configuration groups (primarily used for UI).", responseObject =
ConfigurationGroupResponse.class,
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
since = "4.18.0")
+public class ListCfgGroupsByCmd extends BaseListCmd {
+ public static final Logger s_logger =
Logger.getLogger(ListCfgGroupsByCmd.class.getName());
+
+ public static final String APINAME = "listConfigurationGroups";
+
+ // ///////////////////////////////////////////////////
+ // ////////////// API parameters /////////////////////
+ // ///////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.GROUP, type = CommandType.STRING,
description = "lists configuration group by group name")
+ private String groupName;
+
+ // ///////////////////////////////////////////////////
+ // ///////////////// Accessors ///////////////////////
+ // ///////////////////////////////////////////////////
+
+ public String getGroupName() {
+ return groupName;
+ }
+
+ // ///////////////////////////////////////////////////
+ // ///////////// API Implementation///////////////////
+ // ///////////////////////////////////////////////////
+
+ @Override
+ public String getCommandName() {
+ return APINAME.toLowerCase() + BaseCmd.RESPONSE_SUFFIX;
+ }
+
+ @Override
+ public void execute() {
+ Pair<List<? extends ConfigurationGroup>, Integer> result =
_mgr.listConfigurationGroups(this);
+ ListResponse<ConfigurationGroupResponse> response = new
ListResponse<ConfigurationGroupResponse>();
+ List<ConfigurationGroupResponse> configGroupResponses = new
ArrayList<ConfigurationGroupResponse>();
Review Comment:
```suggestion
ListResponse<ConfigurationGroupResponse> response = new
ListResponse<>();
List<ConfigurationGroupResponse> configGroupResponses = new
ArrayList<>();
```
##########
framework/config/src/test/java/org/apache/cloudstack/framework/config/impl/ConfigDepotAdminTest.java:
##########
@@ -36,11 +41,15 @@
import org.apache.cloudstack.framework.config.ScopedConfigStorage;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import com.cloud.utils.Pair;
+import com.cloud.utils.Ternary;
import com.cloud.utils.db.EntityManager;
public class ConfigDepotAdminTest extends TestCase {
private final static ConfigKey<Integer> DynamicIntCK = new
ConfigKey<Integer>(Integer.class, "dynIntKey", "Advance", "10", "Test Key",
true);
private final static ConfigKey<Integer> StaticIntCK = new
ConfigKey<Integer>(Integer.class, "statIntKey", "Advance", "10", "Test Key",
false);
+ private final static ConfigKey<Integer> TestCK = new
ConfigKey<Integer>(Integer.class, "testKey", "Advance", "30", "Test Key", false,
+ ConfigKey.Scope.Global, null, "Test Display Text", null, new
Ternary<String, String, Long>("TestGroup", "Test Group", 3L), new Pair<String,
Long>("Test SubGroup", 1L));
Review Comment:
```suggestion
ConfigKey.Scope.Global, null, "Test Display Text", null, new
Ternary<>("TestGroup", "Test Group", 3L), new Pair<>("Test SubGroup", 1L));
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/config/ListCfgsByCmd.java:
##########
@@ -145,35 +179,69 @@ public Long getPageSizeVal() {
@Override
public void execute() {
- Pair<List<? extends Configuration>, Integer> result =
_mgr.searchForConfigurations(this);
- ListResponse<ConfigurationResponse> response = new
ListResponse<ConfigurationResponse>();
- List<ConfigurationResponse> configResponses = new
ArrayList<ConfigurationResponse>();
- for (Configuration cfg : result.first()) {
- ConfigurationResponse cfgResponse =
_responseGenerator.createConfigurationResponse(cfg);
- cfgResponse.setObjectName("configuration");
- if (getZoneId() != null) {
- cfgResponse.setScope("zone");
- }
- if (getClusterId() != null) {
- cfgResponse.setScope("cluster");
- }
- if (getStoragepoolId() != null) {
- cfgResponse.setScope("storagepool");
+ validateParameters();
+ try {
+ Pair<List<? extends Configuration>, Integer> result =
_mgr.searchForConfigurations(this);
+ ListResponse<ConfigurationResponse> response = new
ListResponse<ConfigurationResponse>();
+ List<ConfigurationResponse> configResponses = new
ArrayList<ConfigurationResponse>();
+ for (Configuration cfg : result.first()) {
+ ConfigurationResponse cfgResponse =
_responseGenerator.createConfigurationResponse(cfg);
+ if (!matchesConfigurationGroup(cfgResponse)) {
+ continue;
+ }
+ cfgResponse.setObjectName("configuration");
+ if (getZoneId() != null) {
+ cfgResponse.setScope("zone");
+ }
+ if (getClusterId() != null) {
+ cfgResponse.setScope("cluster");
+ }
+ if (getStoragepoolId() != null) {
+ cfgResponse.setScope("storagepool");
+ }
+ if (getAccountId() != null) {
+ cfgResponse.setScope("account");
+ }
+ if (getDomainId() != null) {
+ cfgResponse.setScope("domain");
+ }
+ if (getImageStoreId() != null){
+ cfgResponse.setScope("imagestore");
+ }
+ configResponses.add(cfgResponse);
}
- if (getAccountId() != null) {
- cfgResponse.setScope("account");
+
+ if (StringUtils.isNotEmpty(getGroupName())) {
+ response.setResponses(configResponses, configResponses.size());
+ } else {
+ response.setResponses(configResponses, result.second());
}
- if (getDomainId() != null) {
- cfgResponse.setScope("domain");
+ response.setResponseName(getCommandName());
+ setResponseObject(response);
+ } catch (InvalidParameterValueException e) {
+ throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
e.getMessage());
+ } catch (CloudRuntimeException e) {
+ throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
e.getMessage());
+ }
+ }
+
+ private void validateParameters() {
+ if (StringUtils.isNotEmpty(getSubGroupName()) &&
StringUtils.isEmpty(getGroupName())) {
+ throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"Configuration group name must be specified with the subgroup name");
+ }
+ }
+
+ private boolean matchesConfigurationGroup(ConfigurationResponse
cfgResponse) {
+ if (StringUtils.isNotEmpty(getGroupName())) {
+ if (!(getGroupName().equalsIgnoreCase(cfgResponse.getGroup()))) {
+ return false;
}
- if (getImageStoreId() != null){
- cfgResponse.setScope("imagestore");
+ if (StringUtils.isNotEmpty(getSubGroupName())) {
+ if
(!(getSubGroupName().equalsIgnoreCase(cfgResponse.getSubGroup()))) {
+ return false;
+ }
}
Review Comment:
```suggestion
if (StringUtils.isNotEmpty(getSubGroupName()) &&
!
getSubGroupName().equalsIgnoreCase(cfgResponse.getSubGroup())) {
return false;
}
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/config/ListCfgsByCmd.java:
##########
@@ -145,35 +179,69 @@ public Long getPageSizeVal() {
@Override
public void execute() {
- Pair<List<? extends Configuration>, Integer> result =
_mgr.searchForConfigurations(this);
- ListResponse<ConfigurationResponse> response = new
ListResponse<ConfigurationResponse>();
- List<ConfigurationResponse> configResponses = new
ArrayList<ConfigurationResponse>();
- for (Configuration cfg : result.first()) {
- ConfigurationResponse cfgResponse =
_responseGenerator.createConfigurationResponse(cfg);
- cfgResponse.setObjectName("configuration");
- if (getZoneId() != null) {
- cfgResponse.setScope("zone");
- }
- if (getClusterId() != null) {
- cfgResponse.setScope("cluster");
- }
- if (getStoragepoolId() != null) {
- cfgResponse.setScope("storagepool");
+ validateParameters();
+ try {
+ Pair<List<? extends Configuration>, Integer> result =
_mgr.searchForConfigurations(this);
Review Comment:
again consider if
```suggestion
Pair<List<Configuration>, Integer> result =
_mgr.searchForConfigurations(this);
```
is good enough
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/config/ListCfgsByCmd.java:
##########
@@ -145,35 +179,69 @@ public Long getPageSizeVal() {
@Override
public void execute() {
- Pair<List<? extends Configuration>, Integer> result =
_mgr.searchForConfigurations(this);
- ListResponse<ConfigurationResponse> response = new
ListResponse<ConfigurationResponse>();
- List<ConfigurationResponse> configResponses = new
ArrayList<ConfigurationResponse>();
- for (Configuration cfg : result.first()) {
- ConfigurationResponse cfgResponse =
_responseGenerator.createConfigurationResponse(cfg);
- cfgResponse.setObjectName("configuration");
- if (getZoneId() != null) {
- cfgResponse.setScope("zone");
- }
- if (getClusterId() != null) {
- cfgResponse.setScope("cluster");
- }
- if (getStoragepoolId() != null) {
- cfgResponse.setScope("storagepool");
+ validateParameters();
+ try {
+ Pair<List<? extends Configuration>, Integer> result =
_mgr.searchForConfigurations(this);
+ ListResponse<ConfigurationResponse> response = new
ListResponse<ConfigurationResponse>();
+ List<ConfigurationResponse> configResponses = new
ArrayList<ConfigurationResponse>();
+ for (Configuration cfg : result.first()) {
+ ConfigurationResponse cfgResponse =
_responseGenerator.createConfigurationResponse(cfg);
+ if (!matchesConfigurationGroup(cfgResponse)) {
+ continue;
+ }
+ cfgResponse.setObjectName("configuration");
+ if (getZoneId() != null) {
+ cfgResponse.setScope("zone");
+ }
+ if (getClusterId() != null) {
+ cfgResponse.setScope("cluster");
+ }
+ if (getStoragepoolId() != null) {
+ cfgResponse.setScope("storagepool");
+ }
+ if (getAccountId() != null) {
+ cfgResponse.setScope("account");
+ }
+ if (getDomainId() != null) {
+ cfgResponse.setScope("domain");
+ }
+ if (getImageStoreId() != null){
+ cfgResponse.setScope("imagestore");
+ }
+ configResponses.add(cfgResponse);
}
Review Comment:
can we extract this?
##########
framework/spring/lifecycle/src/main/java/org/apache/cloudstack/spring/lifecycle/registry/ExtensionRegistry.java:
##########
@@ -136,14 +136,17 @@ public ConfigKey<?>[] getConfigKeys() {
List<ConfigKey<String>> result = new ArrayList<ConfigKey<String>>();
if (orderConfigKey != null && orderConfigKeyObj == null) {
- orderConfigKeyObj = new ConfigKey<String>("Advanced",
String.class, orderConfigKey, orderConfigDefault, "The order of precedence for
the extensions", false);
+ orderConfigKeyObj = new ConfigKey<String>(String.class,
orderConfigKey, "Advanced", orderConfigDefault, "The order of precedence for
the extensions", false, ConfigKey.Scope.Global, null, null, null, null, null,
ConfigKey.Kind.Order, orderConfigDefault);
}
+ // orderConfigKeyObj = new ConfigKey<String>("Advanced", String.class,
orderConfigKey, orderConfigDefault, "The order of precedence for the
extensions", false, Scope.Global, null, null, null, null, null, null, null);
+
+
Review Comment:
```suggestion
```
code in comment
##########
framework/config/src/test/java/org/apache/cloudstack/framework/config/impl/ConfigDepotAdminTest.java:
##########
@@ -36,11 +41,15 @@
import org.apache.cloudstack.framework.config.ScopedConfigStorage;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import com.cloud.utils.Pair;
+import com.cloud.utils.Ternary;
import com.cloud.utils.db.EntityManager;
public class ConfigDepotAdminTest extends TestCase {
private final static ConfigKey<Integer> DynamicIntCK = new
ConfigKey<Integer>(Integer.class, "dynIntKey", "Advance", "10", "Test Key",
true);
private final static ConfigKey<Integer> StaticIntCK = new
ConfigKey<Integer>(Integer.class, "statIntKey", "Advance", "10", "Test Key",
false);
+ private final static ConfigKey<Integer> TestCK = new
ConfigKey<Integer>(Integer.class, "testKey", "Advance", "30", "Test Key", false,
Review Comment:
```suggestion
private final static ConfigKey<Integer> TestCK = new
ConfigKey<>(Integer.class, "testKey", "Advance", "30", "Test Key", false,
```
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/manager/ImageStoreProviderManagerImpl.java:
##########
@@ -65,8 +65,8 @@ public class ImageStoreProviderManagerImpl implements
ImageStoreProviderManager,
Map<String, ImageStoreDriver> driverMaps;
- static final ConfigKey<String> ImageStoreAllocationAlgorithm = new
ConfigKey<String>("Advanced", String.class, "image.store.allocation.algorithm",
"firstfitleastconsumed",
- "firstfitleastconsumed','random' : Order in which hosts within a
cluster will be considered for VM/volume allocation", true,
ConfigKey.Scope.Global );
+ static final ConfigKey<String> ImageStoreAllocationAlgorithm = new
ConfigKey<String>(String.class, "image.store.allocation.algorithm", "Advanced",
"firstfitleastconsumed",
Review Comment:
```suggestion
static final ConfigKey<String> ImageStoreAllocationAlgorithm = new
ConfigKey<>(String.class, "image.store.allocation.algorithm", "Advanced",
"firstfitleastconsumed",
```
##########
plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java:
##########
@@ -61,8 +61,8 @@ public interface SAML2AuthManager extends
PluggableAPIAuthenticator, PluggableSe
public static final ConfigKey<String> SAMLDefaultIdentityProviderId = new
ConfigKey<String>("Advanced", String.class, "saml2.default.idpid",
"https://openidp.feide.no";,
"The default IdP entity ID to use only in case of multiple IdPs",
true);
- public static final ConfigKey<String> SAMLSignatureAlgorithm = new
ConfigKey<String>("Advanced", String.class, "saml2.sigalg", "SHA1",
- "The algorithm to use to when signing a SAML request. Default is
SHA1, allowed algorithms: SHA1, SHA256, SHA384, SHA512", true);
+ public static final ConfigKey<String> SAMLSignatureAlgorithm = new
ConfigKey<String>(String.class, "saml2.sigalg", "Advanced", "SHA1",
Review Comment:
```suggestion
public static final ConfigKey<String> SAMLSignatureAlgorithm = new
ConfigKey<>(String.class, "saml2.sigalg", "Advanced", "SHA1",
```
##########
test/integration/smoke/test_global_settings.py:
##########
@@ -75,3 +75,120 @@ def tearDown(self):
updateConfigurationCmd.scopename = "zone"
updateConfigurationCmd.scopeid = 1
self.apiClient.updateConfiguration(updateConfigurationCmd)
+
+class TestListConfigurations(cloudstackTestCase):
+ """
+ Test to list configurations (global settings)
+ """
+ @classmethod
+ def setUpClass(cls):
+ cls.apiclient = cls.testClient.getApiClient()
+ cls._cleanup = []
+
+ @classmethod
+ def tearDownClass(cls):
+ try:
+ cleanup_resources(cls.apiclient, cls._cleanup)
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
+ return
Review Comment:
```suggestion
def tearDownClass(cls):
super(TestListConfigurations, cls).tearDownClass()
```
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -559,19 +563,58 @@ public ServiceOfferingResponse
createServiceOfferingResponse(ServiceOffering off
public ConfigurationResponse createConfigurationResponse(Configuration
cfg) {
ConfigurationResponse cfgResponse = new ConfigurationResponse();
cfgResponse.setCategory(cfg.getCategory());
+ Pair<String, String> configGroupAndSubGroup =
_configMgr.getConfigurationGroupAndSubGroup(cfg.getName());
+ cfgResponse.setGroup(configGroupAndSubGroup.first());
+ cfgResponse.setSubGroup(configGroupAndSubGroup.second());
cfgResponse.setDescription(cfg.getDescription());
cfgResponse.setName(cfg.getName());
if (cfg.isEncrypted()) {
cfgResponse.setValue(DBEncryptionUtil.encrypt(cfg.getValue()));
} else {
cfgResponse.setValue(cfg.getValue());
}
+ cfgResponse.setDefaultValue(cfg.getDefaultValue());
cfgResponse.setIsDynamic(cfg.isDynamic());
+ cfgResponse.setComponent(cfg.getComponent());
+ if (cfg.getParent() != null) {
+ cfgResponse.setParent(cfg.getParent());
+ }
+ cfgResponse.setDisplayText(cfg.getDisplayText());
+ cfgResponse.setType(_configMgr.getConfigurationType(cfg.getName()));
+ if (cfg.getOptions() != null) {
+ cfgResponse.setOptions(cfg.getOptions());
+ }
cfgResponse.setObjectName("configuration");
return cfgResponse;
}
+ @Override
+ public ConfigurationGroupResponse
createConfigurationGroupResponse(ConfigurationGroup cfgGroup) {
+ ConfigurationGroupResponse cfgGroupResponse = new
ConfigurationGroupResponse();
+ cfgGroupResponse.setGroupName(cfgGroup.getName());
+ cfgGroupResponse.setDescription(cfgGroup.getDescription());
+ cfgGroupResponse.setPrecedence(cfgGroup.getPrecedence());
+
+ List<? extends ConfigurationSubGroup> subgroups =
_configMgr.getConfigurationSubGroups(cfgGroup.getId());
Review Comment:
if possible
```suggestion
List<ConfigurationSubGroup> subgroups =
_configMgr.getConfigurationSubGroups(cfgGroup.getId());
```
##########
test/integration/smoke/test_global_settings.py:
##########
@@ -75,3 +75,120 @@ def tearDown(self):
updateConfigurationCmd.scopename = "zone"
updateConfigurationCmd.scopeid = 1
self.apiClient.updateConfiguration(updateConfigurationCmd)
+
+class TestListConfigurations(cloudstackTestCase):
+ """
+ Test to list configurations (global settings)
+ """
+ @classmethod
+ def setUpClass(cls):
+ cls.apiclient = cls.testClient.getApiClient()
+ cls._cleanup = []
+
+ @classmethod
+ def tearDownClass(cls):
+ try:
+ cleanup_resources(cls.apiclient, cls._cleanup)
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
+ return
+
+ def setUp(self):
+ self.apiClient = self.testClient.getApiClient()
+ self.cleanup = []
+
+ def tearDown(self):
+ """
+ Revert any configuration changes
+ """
+ try:
+ cleanup_resources(self.apiclient, self.cleanup)
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
+ return
Review Comment:
```suggestion
super(TestListConfigurations, self).tearDown()
```
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -2214,6 +2250,20 @@ public Pair<List<? extends Configuration>, Integer>
searchForConfigurations(fina
return new Pair<List<? extends Configuration>,
Integer>(result.first(), result.second());
}
+ @Override
+ public Pair<List<? extends ConfigurationGroup>, Integer>
listConfigurationGroups(ListCfgGroupsByCmd cmd) {
+ final Filter searchFilter = new Filter(ConfigurationGroupVO.class,
"precedence", true, null, null);
+ final SearchCriteria<ConfigurationGroupVO> sc =
_configGroupDao.createSearchCriteria();
+
+ final String groupName = cmd.getGroupName();
+ if (StringUtils.isNotBlank(groupName)) {
+ sc.addAnd("name", SearchCriteria.Op.EQ, groupName);
+ }
+
+ final Pair<List<ConfigurationGroupVO>, Integer> result =
_configGroupDao.searchAndCount(sc, searchFilter);
+ return new Pair<List<? extends ConfigurationGroup>,
Integer>(result.first(), result.second());
Review Comment:
```suggestion
return new Pair<List<ConfigurationGroup>, Integer>(result.first(),
result.second());
```
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -2214,6 +2250,20 @@ public Pair<List<? extends Configuration>, Integer>
searchForConfigurations(fina
return new Pair<List<? extends Configuration>,
Integer>(result.first(), result.second());
}
+ @Override
+ public Pair<List<? extends ConfigurationGroup>, Integer>
listConfigurationGroups(ListCfgGroupsByCmd cmd) {
Review Comment:
```suggestion
public Pair<List<ConfigurationGroup>, Integer>
listConfigurationGroups(ListCfgGroupsByCmd cmd) {
```
?
--
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]
