Copilot commented on code in PR #13622:
URL: https://github.com/apache/cloudstack/pull/13622#discussion_r3587170922
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -711,6 +711,8 @@ export default {
})
this.rules = reactive({
name: [{ required: true, message: this.$t('message.error.name') }],
+ // ADD THIS LINE:
+ networkmode: [{ required: true, message:
this.$t('message.error.select') }],
Review Comment:
Remove the leftover instructional comment ("// ADD THIS LINE:")—it’s not
useful in the source and will end up shipped to users reviewing the UI code.
##########
ui/src/views/offering/AddVpcOffering.vue:
##########
@@ -366,6 +366,8 @@ export default {
})
this.rules = reactive({
name: [{ required: true, message: this.$t('message.error.name') }],
+ // ADD THIS LINE:
+ networkmode: [{ required: true, message:
this.$t('message.error.select') }],
Review Comment:
Remove the leftover instructional comment ("// ADD THIS LINE:")—it’s not
useful in the source and will end up shipped to users reviewing the UI code.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/network/NetworkOfferingBaseCmd.java:
##########
@@ -153,11 +153,8 @@ public abstract class NetworkOfferingBaseCmd extends
BaseCmd {
since = "4.20.0")
private Boolean nsxSupportsInternalLbService;
- @Parameter(name = ApiConstants.NETWORK_MODE,
- type = CommandType.STRING,
- description = "Indicates the mode with which the network will
operate. Valid option: NATTED or ROUTED",
- since = "4.20.0")
- private String networkMode;
+ @Parameter(name = ApiConstants.NETWORK_MODE, type = CommandType.STRING,
required = true, description = "the network mode for the network offering")
+ private String networkMode;
Review Comment:
Setting `networkmode` to `required = true` in this shared base command makes
it required for *both* `createNetworkOffering` and `cloneNetworkOffering`
(since both commands extend `NetworkOfferingBaseCmd`). That contradicts
`cloneNetworkOffering`’s API contract (“copied from the source offering unless
explicitly overridden”) and will break clone callers that rely on inheriting
the source offering’s mode.
If the goal is to require `networkmode` only for creation, the required
constraint needs to be applied in `CreateNetworkOfferingCmd` (or via a
create-only base class) rather than on this common superclass.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CreateVPCOfferingCmd.java:
##########
@@ -144,10 +144,7 @@ public class CreateVPCOfferingCmd extends
BaseAsyncCreateCmd {
since = "4.16")
private Boolean enable;
- @Parameter(name = ApiConstants.NETWORK_MODE,
- type = CommandType.STRING,
- description = "Indicates the mode with which the network will
operate. Valid option: NATTED or ROUTED",
- since = "4.20.0")
+ @Parameter(name = ApiConstants.NETWORK_MODE, type = CommandType.STRING,
required = true, description = "the network mode for the VPC offering")
private String networkMode;
Review Comment:
Marking `networkmode` as `required = true` here will also make it required
for `cloneVPCOffering`, because `CloneVPCOfferingCmd` extends
`CreateVPCOfferingCmd` and inherits its parameters. That contradicts the clone
API description (“copied from the source offering unless explicitly
overridden”) and will break existing clone callers that don’t specify
`networkmode`.
To enforce required-ness only for *create* (not clone), the command
hierarchy likely needs to be adjusted (e.g., extract a shared base with
optional `networkmode`, then have `CreateVPCOfferingCmd` add a required
parameter while `CloneVPCOfferingCmd` keeps it optional / defaults from source).
--
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]
