DaanHoogland commented on code in PR #7180:
URL: https://github.com/apache/cloudstack/pull/7180#discussion_r1100072404
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/network/CreateNetworkOfferingCmd.java:
##########
@@ -58,7 +59,7 @@ public class CreateNetworkOfferingCmd extends BaseCmd {
@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required =
true, description = "the name of the network offering")
private String networkOfferingName;
- @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
required = true, description = "the display text of the network offering")
+ @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "the display text of the network offering")
Review Comment:
we could add tath it defaults to `name`, i.e.
```suggestion
@Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "The display text of the network offering, defaults to the value
of 'name'.")
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -59,7 +59,7 @@ public class CreateServiceOfferingCmd extends BaseCmd {
@Parameter(name = ApiConstants.CPU_SPEED, type = CommandType.INTEGER,
required = false, description = "the CPU speed of the service offering in MHz.")
private Integer cpuSpeed;
- @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
required = true, description = "the display text of the service offering")
+ @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "the display text of the service offering")
Review Comment:
```suggestion
@Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "The display text of the service offering, defaults to 'name'.")
```
##########
api/src/main/java/org/apache/cloudstack/api/command/user/iso/RegisterIsoCmd.java:
##########
@@ -55,7 +56,6 @@ public class RegisterIsoCmd extends BaseCmd implements
UserCmd {
@Parameter(name = ApiConstants.DISPLAY_TEXT,
type = CommandType.STRING,
- required = true,
description = "the display text of the ISO. This is usually
used for display purposes.",
Review Comment:
```suggestion
description = "The display text of the ISO, defaults to the
´name´",
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CreateVPCOfferingCmd.java:
##########
@@ -56,7 +57,7 @@ public class CreateVPCOfferingCmd extends BaseAsyncCreateCmd {
@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required =
true, description = "the name of the vpc offering")
private String vpcOfferingName;
- @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
required = true, description = "the display text of " + "the vpc offering")
+ @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "the display text of " + "the vpc offering")
Review Comment:
```suggestion
@Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "the display text of the vpc offering, defaults to the 'name'")
```
##########
api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java:
##########
@@ -67,7 +68,6 @@ public class CreateTemplateCmd extends BaseAsyncCreateCmd
implements UserCmd {
@Parameter(name = ApiConstants.DISPLAY_TEXT,
type = CommandType.STRING,
- required = true,
description = "the display text of the template. This is
usually used for display purposes.",
Review Comment:
```suggestion
description = "The display text of the template, defaults to
the 'name'.",
```
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vpc/CreateVPCCmd.java:
##########
@@ -72,7 +73,7 @@ public class CreateVPCCmd extends BaseAsyncCreateCmd
implements UserCmd {
@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required =
true, description = "the name of the VPC")
private String vpcName;
- @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
required = true, description = "the display text of " +
+ @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "the display text of " +
"the VPC")
Review Comment:
```suggestion
@Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "The display text of the VPC, defaults to its 'name'.")
```
##########
api/src/test/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmdTest.java:
##########
@@ -0,0 +1,41 @@
+// 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.offering;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.test.util.ReflectionTestUtils;
+
+@RunWith(MockitoJUnitRunner.class)
+public class CreateServiceOfferingCmdTest {
+
+ @InjectMocks
+ private CreateServiceOfferingCmd createServiceOfferingCmd;
+
+ @Test
+ public void testGetDisplayTextWhenEmpty() {
+ createServiceOfferingCmd = new CreateServiceOfferingCmd();
Review Comment:
```suggestion
```
##########
test/integration/smoke/test_projects.py:
##########
@@ -1820,3 +1820,135 @@ def test_10_project_activation(self):
"VM should be in Running state after project activation"
)
return
+
+class TestProjectWithEmptyDisplayText(cloudstackTestCase):
+
+ @classmethod
+ def setUpClass(cls):
+ cls.testClient = super(
+ TestProjectWithEmptyDisplayText,
+ cls).getClsTestClient()
+ cls.api_client = cls.testClient.getApiClient()
+
+ cls.services = Services().services
+ # Get Zone
+ cls.zone = get_zone(cls.api_client, cls.testClient.getZoneForTests())
+ cls.hypervisor = cls.testClient.getHypervisorInfo()
+ cls.domain = get_domain(cls.api_client)
+ cls.services['mode'] = cls.zone.networktype
+ cls.template = get_test_template(
+ cls.api_client,
+ cls.zone.id,
+ cls.hypervisor
+ )
+ cls._cleanup = []
+ cls.isGlobalSettingInvalid = False
+ configs = Configurations.list(
+ cls.api_client,
+ name='project.invite.required'
+ )
+
+ if (configs[0].value).lower() != 'false':
+ cls.isGlobalSettingInvalid = True
+ return
+
+ # Create account, service offering, disk offering etc.
+ cls.disk_offering = DiskOffering.create(
+ cls.api_client,
+ cls.services["disk_offering"]
+ )
+ cls._cleanup.append(cls.disk_offering)
+ cls.service_offering = ServiceOffering.create(
+ cls.api_client,
+ cls.services["service_offering"],
+ domainid=cls.domain.id
+ )
+ cls._cleanup.append(cls.service_offering)
+ cls.account = Account.create(
+ cls.api_client,
+ cls.services["account"],
+ admin=True,
+ domainid=cls.domain.id
+ )
+ cls._cleanup.append(cls.account)
+ cls.user = Account.create(
+ cls.api_client,
+ cls.services["account"],
+ admin=True,
+ domainid=cls.domain.id
+ )
+ cls._cleanup.append(cls.user)
+
+ # Create project as a domain admin
+ cls.project = Project.create(
+ cls.api_client,
+ cls.services["project"],
+ account=cls.account.name,
+ domainid=cls.account.domainid
+ )
+ cls._cleanup.append(cls.project)
+ cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+ return
+
+ @classmethod
+ def tearDownClass(cls):
+ try:
+ # Cleanup resources used
+ cleanup_resources(cls.api_client, reversed(cls._cleanup))
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
+ return
+
+ def setUp(self):
+ self.apiclient = self.testClient.getApiClient()
+ self.dbclient = self.testClient.getDbConnection()
+ self.cleanup = []
+
+ if self.isGlobalSettingInvalid:
+ self.skipTest("'project.invite.required' should be set to false")
+ return
+
+ def tearDown(self):
+ try:
+ # Clean up, terminate the created accounts, domains etc
+ cleanup_resources(self.apiclient, reversed(self.cleanup))
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
+ return
Review Comment:
```suggestion
def tearDown(self):
super(TestProjectWithEmptyDisplayText, self).tearDown()
```
##########
api/src/test/java/org/apache/cloudstack/api/command/user/template/RegisterTemplateCmdTest.java:
##########
@@ -139,4 +141,12 @@ public void testIsDeployAsIsNonVmware() {
testIsDeployAsIsBase(Hypervisor.HypervisorType.XenServer, true, false);
testIsDeployAsIsBase(Hypervisor.HypervisorType.Any, true, false);
}
+
+ @Test
+ public void testGetDisplayTextWhenEmpty() {
+ registerTemplateCmd = new RegisterTemplateCmd();
Review Comment:
```suggestion
```
##########
api/src/test/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmdTest.java:
##########
@@ -0,0 +1,41 @@
+// 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.user.project;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.test.util.ReflectionTestUtils;
+
+@RunWith(MockitoJUnitRunner.class)
+public class CreateProjectCmdTest {
+
+ @InjectMocks
+ private CreateProjectCmd createProjectCmd;
+
+ @Test
+ public void testGetDisplayTextWhenEmpty() {
+ createProjectCmd = new CreateProjectCmd();
Review Comment:
```suggestion
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java:
##########
@@ -56,7 +57,7 @@ public class CreateDiskOfferingCmd extends BaseCmd {
@Parameter(name = ApiConstants.DISK_SIZE, type = CommandType.LONG,
required = false, description = "size of the disk offering in GB (1GB =
1,073,741,824 bytes)")
private Long diskSize;
- @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
required = true, description = "alternate display text of the disk offering",
length = 4096)
+ @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "alternate display text of the disk offering", length = 4096)
Review Comment:
in this case the fall back mechanism is implied by 'alternate' :+1:
```suggestion
@Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "An alternate display text of the disk offering.", length = 4096)
```
##########
api/src/main/java/org/apache/cloudstack/api/command/user/project/CreateProjectCmd.java:
##########
@@ -61,7 +62,7 @@ public class CreateProjectCmd extends BaseAsyncCreateCmd {
@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required =
true, description = "name of the project")
private String name;
- @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
required = true, description = "display text of the project")
+ @Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "display text of the project")
Review Comment:
```suggestion
@Parameter(name = ApiConstants.DISPLAY_TEXT, type = CommandType.STRING,
description = "The display text of the project, defaults to the 'name´.")
```
##########
api/src/main/java/org/apache/cloudstack/api/command/user/template/RegisterTemplateCmd.java:
##########
@@ -60,7 +61,6 @@ public class RegisterTemplateCmd extends BaseCmd implements
UserCmd {
@Parameter(name = ApiConstants.DISPLAY_TEXT,
type = CommandType.STRING,
- required = true,
description = "the display text of the template. This is
usually used for display purposes.",
Review Comment:
```suggestion
description = "The display text of the template, defaults to
'name´.",
```
##########
api/src/test/java/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmdTest.java:
##########
@@ -0,0 +1,38 @@
+// 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.offering;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.springframework.test.util.ReflectionTestUtils;
+
+public class CreateDiskOfferingCmdTest {
+
+ @InjectMocks
+ private CreateDiskOfferingCmd createDiskOfferingCmd;
+
+ @Test
+ public void testGetDisplayTextWhenEmpty() {
+ createDiskOfferingCmd = new CreateDiskOfferingCmd();
Review Comment:
I think this line should be deleted
```suggestion
```
##########
api/src/test/java/org/apache/cloudstack/api/command/user/iso/RegisterIsoCmdTest.java:
##########
@@ -0,0 +1,41 @@
+// 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.user.iso;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.test.util.ReflectionTestUtils;
+
+@RunWith(MockitoJUnitRunner.class)
+public class RegisterIsoCmdTest {
+
+ @InjectMocks
+ private RegisterIsoCmd registerIsoCmd;
+
+ @Test
+ public void testGetDisplayTextWhenEmpty() {
+ registerIsoCmd = new RegisterIsoCmd();
Review Comment:
```suggestion
```
##########
api/src/test/java/org/apache/cloudstack/api/command/admin/vpc/CreateVPCOfferingCmdTest.java:
##########
@@ -61,4 +63,12 @@ public void getDetailsNull() throws IllegalArgumentException,
Assert.assertNull(cmd.getServiceProviders());
}
+ @Test
+ public void testCreateVPCOfferingWithEmptyDisplayText() {
+ CreateVPCOfferingCmd cmd = new CreateVPCOfferingCmd();
Review Comment:
in this case it makes sense as no InjectMocks is done on a field by this
name.
##########
test/integration/smoke/test_projects.py:
##########
@@ -1820,3 +1820,135 @@ def test_10_project_activation(self):
"VM should be in Running state after project activation"
)
return
+
+class TestProjectWithEmptyDisplayText(cloudstackTestCase):
+
+ @classmethod
+ def setUpClass(cls):
+ cls.testClient = super(
+ TestProjectWithEmptyDisplayText,
+ cls).getClsTestClient()
+ cls.api_client = cls.testClient.getApiClient()
+
+ cls.services = Services().services
+ # Get Zone
+ cls.zone = get_zone(cls.api_client, cls.testClient.getZoneForTests())
+ cls.hypervisor = cls.testClient.getHypervisorInfo()
+ cls.domain = get_domain(cls.api_client)
+ cls.services['mode'] = cls.zone.networktype
+ cls.template = get_test_template(
+ cls.api_client,
+ cls.zone.id,
+ cls.hypervisor
+ )
+ cls._cleanup = []
+ cls.isGlobalSettingInvalid = False
+ configs = Configurations.list(
+ cls.api_client,
+ name='project.invite.required'
+ )
+
+ if (configs[0].value).lower() != 'false':
+ cls.isGlobalSettingInvalid = True
+ return
+
+ # Create account, service offering, disk offering etc.
+ cls.disk_offering = DiskOffering.create(
+ cls.api_client,
+ cls.services["disk_offering"]
+ )
+ cls._cleanup.append(cls.disk_offering)
+ cls.service_offering = ServiceOffering.create(
+ cls.api_client,
+ cls.services["service_offering"],
+ domainid=cls.domain.id
+ )
+ cls._cleanup.append(cls.service_offering)
+ cls.account = Account.create(
+ cls.api_client,
+ cls.services["account"],
+ admin=True,
+ domainid=cls.domain.id
+ )
+ cls._cleanup.append(cls.account)
+ cls.user = Account.create(
+ cls.api_client,
+ cls.services["account"],
+ admin=True,
+ domainid=cls.domain.id
+ )
+ cls._cleanup.append(cls.user)
+
+ # Create project as a domain admin
+ cls.project = Project.create(
+ cls.api_client,
+ cls.services["project"],
+ account=cls.account.name,
+ domainid=cls.account.domainid
+ )
+ cls._cleanup.append(cls.project)
+ cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+ return
+
+ @classmethod
+ def tearDownClass(cls):
+ try:
+ # Cleanup resources used
+ cleanup_resources(cls.api_client, reversed(cls._cleanup))
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
+ return
Review Comment:
```suggestion
@classmethod
def tearDownClass(cls):
super(TestProjectWithEmptyDisplayText, cls).tearDownClass()
```
##########
server/src/test/java/org/apache/cloudstack/networkoffering/CreateNetworkOfferingTest.java:
##########
@@ -250,4 +257,12 @@ public void createVpcNtwkOffWithNetscaler() {
assertNotNull("Vpc Isolated network offering with Vpc and Netscaler
provider ", off);
}
+ @Test
+ public void createVpcNtwkOffWithEmptyDisplayText() {
+ createNetworkOfferingCmd = new CreateNetworkOfferingCmd();
+ String netName = "network";
+ ReflectionTestUtils.setField(createNetworkOfferingCmd,
"networkOfferingName", netName);
+ Assert.assertEquals(createNetworkOfferingCmd.getDisplayText(),
netName);
+ }
Review Comment:
should this be in a file called `CreateNetworkOfferingCmdTest` instead of
here?
--
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]
