Copilot commented on code in PR #12144:
URL: https://github.com/apache/cloudstack/pull/12144#discussion_r4501217948
##########
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:
##########
@@ -19,6 +19,22 @@
-- Schema upgrade from 4.22.1.0 to 4.23.0.0
--;
+CREATE TABLE IF NOT EXISTS `cloud`.`service_offering_category` (
Review Comment:
This is the only creation of the new category table, but it is not part of
the fresh-install schema (`setup/db/create-schema.sql` has neither
`service_offering_category` nor `service_offering.category_id`), so a new
deployment will fail when the updated service-offering view joins this table.
It is also in the 4.22.1→4.23 migration even though this feature targets 24.0
and the 4.23→24.0 script is `schema-42300to2400.sql`. Add the
table/column/seed/FK to the base schema and the correct upgrade path.
##########
engine/schema/src/main/resources/META-INF/db/views/cloud.service_offering_view.sql:
##########
@@ -85,6 +85,9 @@ SELECT
`vgpu_profile`.`max_resolution_y` AS `vgpu_profile_max_resolution_y`,
`service_offering`.`gpu_count` AS `gpu_count`,
`service_offering`.`gpu_display` AS `gpu_display`,
+ `service_offering`.`category_id` AS `category_id`,
+ `service_offering_category`.`uuid` AS `category_uuid`,
+ `service_offering_category`.`name` AS `category_name`,
Review Comment:
This adds new columns to `service_offering_view`, but the upgrade migration
in this PR never drops and recreates that view. Existing installations will
keep the old view definition, so any `ServiceOfferingJoinVO` query can fail
with unknown `category_id`/`category_uuid`/`category_name` columns (and the new
response fields will never be populated) after upgrade. Add the view
replacement to the appropriate schema migration, not only to the canonical view
resource.
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -9564,4 +9599,97 @@ public void setScope(String scope) {
this.scope = scope;
}
}
+
+ @Override
+ @ActionEvent(eventType =
EventTypes.EVENT_SERVICE_OFFERING_CATEGORY_CREATE, eventDescription = "creating
service offering category")
+ public ServiceOfferingCategory
createServiceOfferingCategory(CreateServiceOfferingCategoryCmd cmd) {
+ String name = cmd.getName();
+ Integer sortKey = cmd.getSortKey();
+
+ // Check if category with same name already exists
+ ServiceOfferingCategoryVO existingCategory =
_serviceOfferingCategoryDao.findByName(name);
+ if (existingCategory != null) {
+ throw new InvalidParameterValueException("Service offering
category with name " + name + " already exists");
+ }
+
+ ServiceOfferingCategoryVO category = new
ServiceOfferingCategoryVO(name);
+ if (sortKey != null) {
+ category.setSortKey(sortKey);
+ }
+
+ category = _serviceOfferingCategoryDao.persist(category);
+ CallContext.current().setEventDetails("Service offering category id="
+ category.getId());
Review Comment:
The new category action events never set an event resource ID or resource
type; they only add text to `eventDetails`. `ActionEventInterceptor` uses the
call-context resource fields (or the annotation) for
`resource_id`/`resource_type`, so create, update, and delete events will be
recorded without a link to the affected category, unlike the existing Guest OS
category operations. Add a resource type for service-offering categories and
populate it with the category ID for these actions.
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -9564,4 +9599,97 @@ public void setScope(String scope) {
this.scope = scope;
}
}
+
+ @Override
+ @ActionEvent(eventType =
EventTypes.EVENT_SERVICE_OFFERING_CATEGORY_CREATE, eventDescription = "creating
service offering category")
+ public ServiceOfferingCategory
createServiceOfferingCategory(CreateServiceOfferingCategoryCmd cmd) {
Review Comment:
These are global category-management operations, but the new CRUD commands
do not declare an admin-only role and these service methods do not perform a
caller-role check. Unlike the existing Guest OS category commands, a role that
is permitted to invoke these APIs can therefore create, update, or delete
categories for the whole cloud. Add the appropriate `RoleType.Admin`
authorization to the management commands and/or enforce it centrally before
persisting.
##########
ui/src/views/compute/wizard/ComputeOfferingSelection.vue:
##########
@@ -17,6 +17,25 @@
<template>
<div>
+ <a-form-item
+ :label="$t('label.category')"
+ name="serviceofferingcategoryid"
+ ref="serviceofferingcategoryid"
+ v-if="serviceOfferingCategories && serviceOfferingCategories.length > 0"
+ style="margin-bottom: 16px;">
+ <block-radio-group-select
+ :maxBlocks="16"
+ :items="serviceOfferingCategories"
+ :selectedValue="selectedServiceOfferingCategoryId"
+ :horizontalGutter="6"
+ :verticalGutter="6"
+ blockSize="medium"
+ @change="handleServiceOfferingCategoryChange">
+ <template #radio-option="{ item }">
+ {{ item.name }}
+ </template>
Review Comment:
`BlockRadioGroupSelect` switches to its `select-option` slot when the
category count exceeds `maxBlocks`, but this caller only supplies
`radio-option`. With 17 or more categories, the dropdown entries therefore have
no rendered labels (and the selected value is not displayed as a name). Provide
a `select-option` slot as well, or avoid using this component for the overflow
case.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/ListServiceOfferingCategoriesCmd.java:
##########
@@ -0,0 +1,71 @@
+// 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.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseListCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.api.response.ServiceOfferingCategoryResponse;
+
+@APICommand(name = "listServiceOfferingCategories",
+ description = "Lists service offering categories.",
+ responseObject = ServiceOfferingCategoryResponse.class,
+ since = "24.0",
+ requestHasSensitiveInfo = false,
Review Comment:
This command has no `authorized` roles, and the PR does not add it to the
default role API mappings. With the dynamic/static RBAC checkers, an API is
allowed only through those mappings or the annotation, so non-root users will
be denied `listServiceOfferingCategories`; the deploy pages then cannot load
categories or filter offerings. Authorize the roles that can list offerings (at
least the roles supported by the deployment wizard), or add the equivalent
default role-permission entries.
--
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]
