Service offering category feature - #12144
Conversation
- Introduced new API commands for creating, updating, and deleting service offering categories. - Added support for associating service offerings with categories. - Updated database schema to include service offering categories. - Enhanced existing service offering commands to handle category IDs.
…e' into service_offering_category_feature
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12144 +/- ##
============================================
+ Coverage 3.53% 21.05% +17.52%
- Complexity 0 20000 +20000
============================================
Files 487 5891 +5404
Lines 41867 534315 +492448
Branches 7913 62611 +54698
============================================
+ Hits 1479 112495 +111016
- Misses 40173 409605 +369432
- Partials 215 12215 +12000
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
Dear @Hanarion, This is a nice feature to have. Are you considering extending it to allow a domain or account to be limited to one (or a specific list) of categories? For example, we distinguish between Core (higher-performance hardware) and Essentials (lower-performance hardware), in a tiered fashion (with different costs, of course). With "Offering Category", we could define which groups of offerings a client (domain/account) is allowed to use. What do you think? |
|
@daviftorres it could be interesting yes. |
Sounds great! |
|
@blueorangutan package |
|
@rajujith a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15873 |
|
@rajujith thanks for the review. I'll check today in order to fix this issue |
Screen.Recording.2025-12-03.at.10.03.17.AM.mov |
…on/cloudstack into service_offering_category_feature
…breaking functionnality
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated 2 comments.
Suppressed comments (8)
ui/src/views/compute/wizard/ComputeOfferingSelection.vue:1
- The newly added style block targets
.radio-option(and nested elements), but the slot template renders only text and never applies the.radio-optionclass. This makes the CSS dead/unreachable. Either wrap the slot contents in an element withclass=\"radio-option\"(and corresponding nested structure) or delete the unused styles.
ui/src/views/compute/wizard/ComputeOfferingSelection.vue:1 - The newly added style block targets
.radio-option(and nested elements), but the slot template renders only text and never applies the.radio-optionclass. This makes the CSS dead/unreachable. Either wrap the slot contents in an element withclass=\"radio-option\"(and corresponding nested structure) or delete the unused styles.
ui/src/components/offering/ComputeOfferingForm.vue:1 - This API call has no
.catch(...). IfgetAPI('listServiceOfferingCategories')rejects, it can surface as an unhandled promise rejection and the UI won’t have a defined fallback list. Add a catch handler (e.g., setthis.categories = []and optionally show a notification) so failures are handled gracefully.
ui/src/views/compute/DeployVM.vue:1 - This wraps
getAPI(...)(already a Promise) in a new Promise, which adds complexity and makes error flow harder to follow. Prefer returning the existing promise chain directly (and let callersawaitit if needed). This pattern is duplicated in DeployVnfAppliance.vue as well, so simplifying it once and reusing shared logic would reduce maintenance.
ui/src/views/compute/DeployVnfAppliance.vue:1 - Same issue as DeployVM.vue: unnecessary
new Promise(...)wrapping and duplicated implementation. Consider extracting a shared helper (mixin/composable/util) used by both deploy flows to keep behavior consistent (including fallback/default option) and reduce duplication.
ui/src/components/view/ListView.vue:1 - Using
$route.path.split('/')[1] === 'computeoffering'is brittle (path structure changes, leading/trailing slashes, nested routes). Prefer checking$route.name, a route meta flag, or the current section/entity name if available. That keeps the linking logic stable across routing refactors.
server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java:1 - Leaving test mocks with TODO stubs returning null/false can cause hard-to-debug NPEs if any test path starts exercising the new APIs. Prefer either (a) throwing UnsupportedOperationException with a clear message so failures are explicit, or (b) providing a minimal in-memory implementation suitable for tests.
server/src/test/java/com/cloud/configuration/ConfigurationManagerCloneIntegrationTest.java:1 - Tests were updated to accommodate the new categoryId parameter in service offering creation/cloning, but there are no tests shown validating the new category behaviors (e.g., create/update/delete category, list categories, filtering listServiceOfferings by categoryid, and cloning inheriting category). Adding coverage for these cases would help prevent regressions in the new feature.
| CREATE TABLE IF NOT EXISTS `cloud`.`service_offering_category` ( | ||
| `id` bigint unsigned NOT NULL auto_increment, | ||
| `name` varchar(255) NOT NULL, | ||
| `uuid` varchar(40), | ||
| `sort_key` int NOT NULL DEFAULT 0, | ||
| PRIMARY KEY (`id`), | ||
| CONSTRAINT `uc_service_offering_category__uuid` UNIQUE (`uuid`), | ||
| CONSTRAINT `uc_service_offering_category__name` UNIQUE (`name`) | ||
| ) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8; | ||
|
|
||
| INSERT INTO `cloud`.`service_offering_category` (id, name, uuid) VALUES (1, 'Default', UUID()); | ||
|
|
||
| ALTER TABLE `cloud`.`service_offering` ADD COLUMN `category_id` bigint unsigned NOT NULL DEFAULT 1; | ||
| ALTER TABLE `cloud`.`service_offering` ADD CONSTRAINT `fk_service_offering__category_id` FOREIGN KEY (`category_id`) REFERENCES `cloud`.`service_offering_category` (`id`) ON DELETE RESTRICT ON UPDATE CASCADE; |
| // Check if any service offering is using this category | ||
| // For now we'll just check if it's the default category (id=1) | ||
| if (categoryId == 1L) { | ||
| throw new InvalidParameterValueException("Cannot delete the default service offering category"); | ||
| } |
|
@Hanarion , are you still interested? |
…ring/CreateServiceOfferingCmd.java Co-authored-by: dahn <daan.hoogland@gmail.com>
…ring/CreateServiceOfferingCategoryCmd.java Co-authored-by: dahn <daan.hoogland@gmail.com>
| @APICommand(name = "deleteServiceOfferingCategory", | ||
| description = "Deletes a service offering category.", | ||
| responseObject = SuccessResponse.class, | ||
| since = "4.23.0", |
There was a problem hiding this comment.
there are several places @Hanarion , please go through the PR
There was a problem hiding this comment.
🟡 Changes recommended
It introduces several correctness issues (API version annotations, duplicated UI config keys overriding behavior, and robustness/permission handling gaps) that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
Previously missed (3) — in code that hasn't changed since the last review.
ui/src/config/section/offering.js:46
- The object defines
filterstwice; the laterfilters: ['active', 'inactive']overrides the role-basedfilters()function, so non-admin users will incorrectly see the admin filters (and any future changes tofilters()will be ignored). Remove the duplicatefiltersproperty and keep a single definition.
ui/src/views/compute/DeployVM.vue:2342 - This always calls
listServiceOfferingCategoriesduring VM deploy. For roles that don’t have the API, this will consistently log an error and show a meaningless single "All" option. Guard the call with an API-availability check and initialize the selected category to-1when categories are loaded.
ui/src/views/compute/DeployVnfAppliance.vue:2714 - This always calls
listServiceOfferingCategorieson load; if the current role doesn’t have that API, it will always hit the error path and log to console. Add an API-availability guard to avoid noisy errors and to skip the call when unsupported.
ui/src/views/compute/wizard/ComputeOfferingSelection.vue:36
- The new
.radio-optionstyles won’t apply because the slot content doesn’t render an element withclass="radio-option"; currently it only outputs text. Wrap the slot content so the CSS is actually used (and so long names can be ellipsized as intended).
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:298 - The new
categoryidparameter is marked assince = "24.0", but this PR’s schema upgrade targets 4.23.0.0 and related commands usesince = "4.23.0". This should be4.23.0for consistent API versioning/annotations.
entityType = ServiceOfferingCategoryResponse.class,
required = false,
description = "the ID of the service offering category to associate with this offering",
since = "24.0")
private Long categoryId;
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:35
- The migration inserts the default category with an unconditional
INSERT ... VALUES (1, ...), which will fail if the upgrade script is re-run (or if the table already exists for any reason). Also,uuidis nullable even though it’s used as the API identifier (unique constraints allow multiple NULLs in MySQL). MakeuuidNOT NULL and guard the default-row insert so upgrades remain robust.
CREATE TABLE IF NOT EXISTS `cloud`.`service_offering_category` (
`id` bigint unsigned NOT NULL auto_increment,
`name` varchar(255) NOT NULL,
`uuid` varchar(40),
`sort_key` int NOT NULL DEFAULT 0,
PRIMARY KEY (`id`),
CONSTRAINT `uc_service_offering_category__uuid` UNIQUE (`uuid`),
CONSTRAINT `uc_service_offering_category__name` UNIQUE (`name`)
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8;
INSERT INTO `cloud`.`service_offering_category` (id, name, uuid) VALUES (1, 'Default', UUID());
ALTER TABLE `cloud`.`service_offering` ADD COLUMN `category_id` bigint unsigned NOT NULL DEFAULT 1;
ALTER TABLE `cloud`.`service_offering` ADD CONSTRAINT `fk_service_offering__category_id` FOREIGN KEY (`category_id`) REFERENCES `cloud`.`service_offering_category` (`id`) ON DELETE RESTRICT ON UPDATE CASCADE;
- Files reviewed: 43/43 changed files
- Comments generated: 1
- Review effort level: Lite
| @APICommand(name = "createServiceOfferingCategory", | ||
| description = "Creates a service offering category.", | ||
| responseObject = ServiceOfferingCategoryResponse.class, | ||
| since = "24.0", | ||
| requestHasSensitiveInfo = false, | ||
| responseHasSensitiveInfo = false) |
|
@DaanHoogland yes still interested I just pushed a commit applying your 24.0 suggestion to the rest of the category API so it's consistent, and went through the remaining copilot comments : clone was ignoring the categoryid given on the command, the schema upgrade is now idempotent, and i fixed the missing category state in DeployVM.vue. I also added unit tests on the category validation. |
There was a problem hiding this comment.
🟡 Changes recommended
Several introduced API/version metadata and UI/API-availability handling issues should be corrected to avoid inaccurate API docs and runtime errors for users lacking the new API permission.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:30
service_offering_category.uuidis nullable, but the object implementsIdentityand the table enforces a UNIQUE constraint. In MySQL, UNIQUE allows multiple NULLs, so this doesn’t actually guarantee uniqueness unless the column is NOT NULL. Consider makinguuidNOT NULL to enforce invariants at the DB layer.
api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java:295
- The new category fields are marked with
since = "24.0", which doesn’t match the CloudStack version format used elsewhere in this codebase (e.g.4.22.0,4.23.0). This affects API documentation/versioning metadata; please update to the correct release version for this feature (likely4.23.0).
@SerializedName("categoryid")
@Param(description = "the ID of the service offering category", since = "24.0")
private String categoryId;
@SerializedName("category")
@Param(description = "the name of the service offering category", since = "24.0")
private String categoryName;
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCategoryCmd.java:35
since = "24.0"is inconsistent with the CloudStack API version strings used throughout the project (e.g.4.23.0). Update this to the correct CloudStack release version for the new command so API docs remain accurate.
@APICommand(name = "createServiceOfferingCategory",
description = "Creates a service offering category.",
responseObject = ServiceOfferingCategoryResponse.class,
since = "24.0",
requestHasSensitiveInfo = false,
responseHasSensitiveInfo = false)
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:298
- The new
categoryidparameter is annotated withsince = "24.0", which doesn’t match the CloudStack version format used elsewhere. Please update to the correct release version (likely4.23.0) so the API metadata stays consistent.
@Parameter(name = ApiConstants.SERVICE_OFFERING_CATEGORY_ID,
type = CommandType.UUID,
entityType = ServiceOfferingCategoryResponse.class,
required = false,
description = "the ID of the service offering category to associate with this offering",
since = "24.0")
private Long categoryId;
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/UpdateServiceOfferingCmd.java:118
- The new
categoryidparameter is annotated withsince = "24.0", which doesn’t match the CloudStack version format used elsewhere. Please update to the correct release version (likely4.23.0) so the API metadata stays consistent.
@Parameter(name = ApiConstants.SERVICE_OFFERING_CATEGORY_ID,
type = CommandType.UUID,
entityType = ServiceOfferingCategoryResponse.class,
required = false,
description = "the ID of the service offering category to associate",
since = "24.0")
private Long categoryId;
ui/src/views/compute/wizard/ComputeOfferingSelection.vue:36
- The added
.radio-option/.ellipsisstyles aren’t applied because the slot content only renders{{ item.name }}. As-is, these new styles are dead code and long names won’t ellipsize. Either remove the unused CSS or wrap the slot content to use the classes.
- Files reviewed: 44/44 changed files
- Comments generated: 7
- Review effort level: Lite
| <template v-if="column.key === 'category' && $route.path.split('/')[1] === 'computeoffering'"> | ||
| <span v-if="record.categoryid"> | ||
| <router-link :to="{ path: '/serviceofferingcategory/' + record.categoryid }">{{ text }}</router-link> | ||
| </span> | ||
| <span v-else>{{ text }}</span> | ||
| </template> |
| fetchServiceOfferingCategories () { | ||
| this.loading.serviceOfferingCategories = true | ||
| return new Promise((resolve, reject) => { | ||
| getAPI('listServiceOfferingCategories').then(json => { |
| fetchServiceOfferingCategories () { | ||
| this.loading.serviceOfferingCategories = true | ||
| return new Promise((resolve, reject) => { | ||
| getAPI('listServiceOfferingCategories').then(json => { |
| @APICommand(name = "deleteServiceOfferingCategory", | ||
| description = "Deletes a service offering category.", | ||
| responseObject = SuccessResponse.class, | ||
| since = "24.0", | ||
| requestHasSensitiveInfo = false, | ||
| responseHasSensitiveInfo = false) |
| @APICommand(name = "listServiceOfferingCategories", | ||
| description = "Lists service offering categories.", | ||
| responseObject = ServiceOfferingCategoryResponse.class, | ||
| since = "24.0", | ||
| requestHasSensitiveInfo = false, | ||
| responseHasSensitiveInfo = false) |
| @APICommand(name = "updateServiceOfferingCategory", | ||
| description = "Updates a service offering category", | ||
| responseObject = ServiceOfferingCategoryResponse.class, | ||
| since = "24.0", | ||
| requestHasSensitiveInfo = false, | ||
| responseHasSensitiveInfo = false) |
| @Parameter(name = ApiConstants.SERVICE_OFFERING_CATEGORY_ID, | ||
| type = CommandType.UUID, | ||
| entityType = ServiceOfferingCategoryResponse.class, | ||
| description = "the ID of the service offering category", | ||
| since = "24.0") | ||
| private Long categoryId; | ||
|
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 4 comments.
Suppressed comments (3)
ui/src/components/offering/ComputeOfferingForm.vue:1
fetchCategories()unconditionally callslistServiceOfferingCategories. In the same PR, other UI flows explicitly guard this call when the API isn’t available (older management server / plugin set), but this form doesn’t—so the create/edit offering UI can break in environments where the new API command isn’t registered. Add an API-availability guard (like checking('listServiceOfferingCategories' in this.$store.getters.apis)) and handle failures by keepingcategories = [](and optionally hiding the selector).
ui/src/components/offering/ComputeOfferingForm.vue:1fetchCategories()unconditionally callslistServiceOfferingCategories. In the same PR, other UI flows explicitly guard this call when the API isn’t available (older management server / plugin set), but this form doesn’t—so the create/edit offering UI can break in environments where the new API command isn’t registered. Add an API-availability guard (like checking('listServiceOfferingCategories' in this.$store.getters.apis)) and handle failures by keepingcategories = [](and optionally hiding the selector).
server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java:1- These new mock methods return
null/falsewith TODOs. That can cause hard-to-debug NPEs if a test starts invoking them indirectly. Prefer throwingUnsupportedOperationException(fails fast) or implementing a minimal deterministic stub consistent with existing mock behavior.
| Long id = cmd.getId(); | ||
| String name = cmd.getName(); | ||
|
|
||
| Filter searchFilter = new Filter(ServiceOfferingCategoryVO.class, "sortKey", true, cmd.getStartIndex(), cmd.getPageSizeVal()); |
|
|
||
| Integer getGpuCount(); | ||
|
|
||
| long getCategoryId(); |
|
|
||
| CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.service_offering', 'category_id', 'bigint unsigned NOT NULL DEFAULT 1'); | ||
| CALL `cloud`.`IDEMPOTENT_DROP_FOREIGN_KEY`('cloud.service_offering', 'fk_service_offering__category_id'); | ||
| ALTER TABLE `cloud`.`service_offering` ADD CONSTRAINT `fk_service_offering__category_id` FOREIGN KEY (`category_id`) REFERENCES `cloud`.`service_offering_category` (`id`) ON DELETE RESTRICT ON UPDATE CASCADE; |
| public static final String SENT_BYTES = "sentbytes"; | ||
| public static final String SERIAL = "serial"; | ||
| public static final String SERVICE_IP = "serviceip"; | ||
| public static final String SERVICE_OFFERING_CATEGORY_ID = "categoryid"; |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 3 comments.
Suppressed comments (4)
ui/src/components/offering/ComputeOfferingForm.vue:1
- The category
<a-select-option>does not set an explicit:value, which can result in the selectedform.categoryidbeingundefinedor not matching the intended category ID depending on Ant Design Vue behavior. Also,fetchCategories()unconditionally callslistServiceOfferingCategorieswithout checking API availability (unlike DeployVM/DeployVnfAppliance), which will error against older backends; add the same API-existence guard and handle rejected promises (e.g., set empty list and stop loading).
ui/src/components/offering/ComputeOfferingForm.vue:1 - The category
<a-select-option>does not set an explicit:value, which can result in the selectedform.categoryidbeingundefinedor not matching the intended category ID depending on Ant Design Vue behavior. Also,fetchCategories()unconditionally callslistServiceOfferingCategorieswithout checking API availability (unlike DeployVM/DeployVnfAppliance), which will error against older backends; add the same API-existence guard and handle rejected promises (e.g., set empty list and stop loading).
server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java:1 - These mock methods silently return
null/false, which can mask errors (or cause later NPEs) if any test hits these paths. In test mocks, prefer failing fast withUnsupportedOperationException(or implement minimal deterministic behavior) so unintended calls are immediately visible.
ui/src/views/compute/DeployVM.vue:1 - This category-fetching logic is duplicated (also in DeployVnfAppliance.vue) with identical behavior and error handling. Consider extracting a shared helper/mixin/composable (or a store action) to avoid divergence and keep UI behavior consistent when the API contract changes.
| CREATE TABLE IF NOT EXISTS `cloud`.`service_offering_category` ( | ||
| `id` bigint unsigned NOT NULL auto_increment, | ||
| `name` varchar(255) NOT NULL, | ||
| `uuid` varchar(40), | ||
| `sort_key` int NOT NULL DEFAULT 0, | ||
| PRIMARY KEY (`id`), | ||
| CONSTRAINT `uc_service_offering_category__uuid` UNIQUE (`uuid`), | ||
| CONSTRAINT `uc_service_offering_category__name` UNIQUE (`name`) | ||
| ) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8; |
| // Check if any service offering is using this category | ||
| // For now we'll just check if it's the default category (id=1) | ||
| if (categoryId == 1L) { | ||
| throw new InvalidParameterValueException("Cannot delete the default service offering category"); | ||
| } |
| public static final String SENT_BYTES = "sentbytes"; | ||
| public static final String SERIAL = "serial"; | ||
| public static final String SERVICE_IP = "serviceip"; | ||
| public static final String SERVICE_OFFERING_CATEGORY_ID = "categoryid"; |
Description
When using Cloudstack, when creating instance with a lot of offerings, it could be hard to differentiate between all of the offerings. For that, categories could be useful.
This pull request introduces the concept of "Service Offering Categories" to the API, allowing service offerings to be grouped, managed, and queried by category. It adds a new interface for categories, updates the API to support creating, updating, deleting, and listing categories, and enables associating service offerings with a category. The changes also extend existing API commands and responses to work with categories.
Service Offering Category API Support:
CreateServiceOfferingCategoryCmd,DeleteServiceOfferingCategoryCmd,UpdateServiceOfferingCategoryCmd, andListServiceOfferingCategoriesCmd, enabling full CRUD operations and listing for categories.ConfigurationServiceinterface to include methods for creating, deleting, and updating service offering categories.ServiceOfferingCategorythat defines category properties and behaviors.API Parameter and Response Enhancements:
SERVICE_OFFERING_CATEGORY_ID,SERVICE_OFFERING_CATEGORY_NAME) inApiConstants, and updated related commands (CreateServiceOfferingCmd,UpdateServiceOfferingCmd,ListServiceOfferingsCmd) to accept or filter by category.ResponseGeneratorto support generating responses for service offering categories.Service Offering Model Update:
getCategoryId()method to theServiceOfferinginterface, allowing offerings to be associated with a specific category.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
On my dev environment, through the API and cloudmonkey
How did you try to break this feature and the system with this change?
Those changes should not break any features as it is only adding a new column to serviceoffering and adding a new table, it is only a way to filter and categorize.
--
I'm sorry if the formatting isn't perfect, i couldn't get the pre-commit to work, and it is my first code PR.
Warning, i put the SQL where i thought it made sense, but i think you will want to move it where it really should be.