Copilot commented on code in PR #12864:
URL: https://github.com/apache/cloudstack/pull/12864#discussion_r2964583551
##########
ui/src/views/network/NicsTable.vue:
##########
@@ -39,7 +39,7 @@
{{ record.traffictype }}
</a-descriptions-item>
<a-descriptions-item :label="$t('label.secondaryips')"
v-if="record.secondaryip && record.secondaryip.length > 0 && record.type !==
'L2'">
- {{ record.secondaryip.map(x => x.ipaddress).join(', ') }}
+ {{ record.secondaryip.map(x => x.ipaddress + ': ' + x.description
).join(', ') }}
Review Comment:
`x.description` is optional (DB default is NULL), so this rendering can show
`ip: undefined/null` for existing secondary IPs without a description. Consider
only appending `: <description>` when a non-empty description is present (or
fall back to just the IP).
```suggestion
{{ record.secondaryip.map(x => x.description ? (x.ipaddress + ': '
+ x.description) : x.ipaddress).join(', ') }}
```
##########
ui/src/views/network/NicsTab.vue:
##########
@@ -215,6 +215,11 @@
:placeholder="$t('label.new.secondaryip.description')"
v-model:value="newSecondaryIp"
v-focus="editNicResource.type!=='Shared'"></a-input>
+ <p class="modal-form__label">{{ $t('label.secondaryip.description')
}}</p>
+ <a-input
+ :placeholder="$t('label.new.secondaryip.description.description')"
+ v-model:value="newSecondaryIpDescription"
+ v-focus="editNicResource.type!=='Shared'"></a-input>
Review Comment:
Both the secondary IP input and the new description input have
`v-focus="editNicResource.type!=='Shared'"`. Since the directive focuses
elements on mount, the second input will steal focus from the first one, making
the modal default focus the description field instead of the IP field. Consider
removing `v-focus` from the description input (or making focus conditional so
only one element gets it).
```suggestion
v-model:value="newSecondaryIpDescription"></a-input>
```
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vm/AddIpToVmNicCmd.java:
##########
@@ -56,6 +56,9 @@ public class AddIpToVmNicCmd extends BaseAsyncCreateCmd {
@Parameter(name = ApiConstants.IP_ADDRESS, type = CommandType.STRING,
required = false, description = "Secondary IP Address")
private String ipAddr;
+ @Parameter(name = ApiConstants.DESCRIPTION, type = CommandType.STRING,
required = false, description = "Description")
Review Comment:
The new `description` API parameter is persisted into a `varchar(2048)`
column, but the command parameter does not specify a length limit. Consider
adding `length = 2048` (and optionally trimming/validating) to avoid DB errors
when a longer string is provided.
```suggestion
@Parameter(name = ApiConstants.DESCRIPTION, type = CommandType.STRING,
required = false, description = "Description", length = 2048)
```
##########
api/src/test/java/org/apache/cloudstack/api/command/test/AddIpToVmNicTest.java:
##########
@@ -59,7 +59,7 @@ public void testCreateSuccess() throws
ResourceAllocationException, ResourceUnav
NicSecondaryIp secIp = Mockito.mock(NicSecondaryIp.class);
Mockito.when(
-
networkService.allocateSecondaryGuestIP(ArgumentMatchers.anyLong(),
ArgumentMatchers.any()))
+
networkService.allocateSecondaryGuestIP(ArgumentMatchers.anyLong(),
ArgumentMatchers.any(), ArgumentMatchers.anyString()))
.thenReturn(secIp);
Review Comment:
The updated tests adjust the mocked method signature, but they still don't
assert that the new `description` value is actually passed through to
`NetworkService.allocateSecondaryGuestIP(...)`. Adding a verification (e.g.,
`verify(...)` with an expected description or an `ArgumentCaptor`) would ensure
the new feature is covered and prevents regressions.
```suggestion
// Explicitly call allocateSecondaryGuestIP with a known description
and verify it is passed through.
networkService.allocateSecondaryGuestIP(1L, null, "testDescription");
Mockito.verify(networkService).allocateSecondaryGuestIP(ArgumentMatchers.anyLong(),
ArgumentMatchers.any(), ArgumentMatchers.eq("testDescription"));
```
##########
server/src/main/java/com/cloud/network/rules/RulesManagerImpl.java:
##########
@@ -467,7 +467,7 @@ private boolean enableStaticNat(long ipId, long vmId, long
networkId, boolean is
boolean isOneToOneNat = ipAddress.isOneToOneNat();
Long associatedWithVmId = ipAddress.getAssociatedWithVmId();
Nic guestNic;
- NicSecondaryIpVO nicSecIp = null;
+ wNicSecondaryIpVO nicSecIp = null;
Review Comment:
`wNicSecondaryIpVO` is not a known type (and `NicSecondaryIpVO` is already
imported). This looks like an accidental rename and will cause a compilation
failure; the variable should use `NicSecondaryIpVO` (or the correct intended
class name).
```suggestion
NicSecondaryIpVO nicSecIp = null;
```
--
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]
