You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2023/01/05 20:26:27 UTC
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #5797: Improve global settings UI to be more intuitive/logical
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org