Add support to clone existing offerings and update them#12357
Add support to clone existing offerings and update them#12357
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12357 +/- ##
============================================
+ Coverage 17.89% 17.97% +0.07%
- Complexity 16092 16284 +192
============================================
Files 5936 5950 +14
Lines 532734 534978 +2244
Branches 65165 65703 +538
============================================
+ Hits 95347 96157 +810
- Misses 426711 427999 +1288
- Partials 10676 10822 +146
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pearl1594
force-pushed
the
clone-edit-existing-offerings
branch
from
62368f3 to
3787aaa
Compare
Pearl1594
force-pushed
the
clone-edit-existing-offerings
branch
from
a5a9eb4 to
bb2e90d
Compare
Pearl1594
force-pushed
the
clone-edit-existing-offerings
branch
from
bb2e90d to
7fd4567
Compare
|
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.
could you make the new if and for blocks separte methods (with clear names please?
…e-edit-existing-offerings
Pearl1594
force-pushed
the
clone-edit-existing-offerings
branch
from
a7266b2 to
cc4ada0
Compare
Pearl1594
force-pushed
the
clone-edit-existing-offerings
branch
from
5f5147d to
3511525
Compare
Pearl1594
force-pushed
the
clone-edit-existing-offerings
branch
from
1f5796d to
10333df
Compare
…in add/clone operations
…e-edit-existing-offerings
…tris from the source networkmode
|
weizhouapache
requested review from
Copilot and
weizhouapache
and removed request for
ustcweizhou
There was a problem hiding this comment.
Pull request overview
This PR introduces “clone offering” functionality across the CloudStack API and UI, enabling users to clone existing offerings (compute/service, disk, network, VPC, backup) while overriding selected parameters. It also refactors network/VPC service-capability param building into shared UI helpers and wires the new actions into the offerings UI.
Changes:
- Added new API commands and server implementations for cloning multiple offering types (plus event types and service interfaces).
- Added new UI clone views (compute/disk/backup shown here) and integrated clone actions into the offerings section config.
- Refactored network/VPC service-capability parameter construction into a shared composable and extracted disk offering form into a reusable component.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/offering/CloneDiskOffering.vue | New UI flow to clone disk offerings using the shared DiskOfferingForm. |
| ui/src/views/offering/CloneComputeOffering.vue | New UI flow to clone compute/service offerings (includes prefill + submit logic). |
| ui/src/views/offering/CloneBackupOffering.vue | New UI flow to clone backup offerings (zone/provider offering selection + async job polling). |
| ui/src/views/offering/AddVpcOffering.vue | Refactors VPC service capability param building into a shared helper. |
| ui/src/views/offering/AddNetworkOffering.vue | Refactors network service capability param building into a shared helper. |
| ui/src/views/offering/AddDiskOffering.vue | Refactors disk offering creation to use the new shared DiskOfferingForm component. |
| ui/src/config/section/offering.js | Adds new “Clone * Offering” actions in the UI offerings sections. |
| ui/src/composables/useServiceCapabilityParams.js | New shared helper(s) to build serviceCapability/serviceProvider param sets for network/VPC offerings. |
| ui/src/components/offering/DiskOfferingForm.vue | New reusable disk offering form component extracted from the previous view. |
| ui/src/components/CheckBoxSelectPair.vue | Adds support for setting a default select value. |
| ui/public/locales/en.json | Adds i18n strings for clone offering actions and messages. |
| server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java | Adds tests asserting cloned backup offering domain handling. |
| server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java | Updates test mock to support new clone APIs and new base cmd type for network offering creation. |
| server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java | Adds tests around reflection helpers and some clone-related scaffolding. |
| server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java | Implements backup offering clone behavior and registers the command. |
| server/src/main/java/com/cloud/server/ManagementServerImpl.java | Registers new clone commands in management server command list. |
| server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java | Implements VPC offering clone logic, including service/provider/capability reconstruction. |
| api/src/test/java/org/apache/cloudstack/api/command/admin/vpc/CloneVpcOfferingCmdTest.java | Adds unit tests for the new clone VPC offering command. |
| api/src/test/java/org/apache/cloudstack/api/command/admin/offering/CloneServiceOfferingCmdTest.java | Adds unit tests for the new clone service offering command. |
| api/src/test/java/org/apache/cloudstack/api/command/admin/network/CloneNetworkOfferingCmdTest.java | Adds unit tests for the new clone network offering command. |
| api/src/test/java/org/apache/cloudstack/api/command/admin/backup/CloneBackupOfferingCmdTest.java | Adds unit tests for the new clone backup offering command. |
| api/src/main/java/org/apache/cloudstack/backup/BackupManager.java | Adds cloneBackupOffering to the backup manager interface. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CreateVPCOfferingCmd.java | Adjusts supported-services behavior for external providers (validation moved server-side). |
| api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CloneVPCOfferingCmd.java | New API command for cloning VPC offerings. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CloneServiceOfferingCmd.java | New API command for cloning service offerings. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CloneDiskOfferingCmd.java | New API command for cloning disk offerings. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/network/NetworkOfferingBaseCmd.java | New base command consolidating shared network-offering params/logic. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/network/CreateNetworkOfferingCmd.java | Refactors create network offering cmd to extend NetworkOfferingBaseCmd. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/network/CloneNetworkOfferingCmd.java | New API command for cloning network offerings. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/backup/ImportBackupOfferingCmd.java | Makes backupManager protected and annotates domainIds param with since. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/backup/CloneBackupOfferingCmd.java | New API command for cloning backup offerings (domainIds resolver support). |
| api/src/main/java/org/apache/cloudstack/api/ApiConstants.java | Adds SOURCE_OFFERING_ID constant. |
| api/src/main/java/com/cloud/network/vpc/VpcProvisioningService.java | Adds cloneVPCOffering to VPC provisioning service interface. |
| api/src/main/java/com/cloud/event/EventTypes.java | Adds new event types for offering/backup offering clone. |
| api/src/main/java/com/cloud/configuration/ConfigurationService.java | Adds clone methods and generalizes createNetworkOffering to accept the new base cmd. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @APICommand(name = "cloneBackupOffering", | ||
| description = "Clones a backup offering from an existing offering", | ||
| responseObject = BackupOfferingResponse.class, since = "4.14.0", |
There was a problem hiding this comment.
The API annotation says since = "4.14.0", but this clone API is introduced alongside the other clone APIs marked since = "4.23.0" and even has a parameter marked since = "4.23.0". Consider aligning the command-level since value to the actual release where this API is added.
| responseObject = BackupOfferingResponse.class, since = "4.14.0", | |
| responseObject = BackupOfferingResponse.class, since = "4.23.0", |
| export default { | ||
| name: 'CreateComputeOffering', | ||
| mixins: [mixinForm], | ||
| components: { |
There was a problem hiding this comment.
The component is registered with name 'CreateComputeOffering', but this view is for cloning. This breaks devtools/component identification and may affect caching/keep-alive behavior if other components rely on the name. Rename it to 'CloneComputeOffering' (or another clone-specific name).
| } | ||
|
|
||
| params.sourceofferingid = this.resource.id | ||
|
|
||
| postAPI('cloneServiceOffering', params).then(json => { | ||
| const message = this.isSystem |
There was a problem hiding this comment.
this.loading is never set to true before calling postAPI('cloneServiceOffering', ...), so the submit-guard (if (this.loading) return) does not prevent double submits and the UI spinner never activates. Set this.loading = true immediately before the API call (and keep the existing finally that resets it).
| required = true, description = "The ID of the source backup offering to clone from") | ||
| private Long sourceOfferingId; | ||
|
|
||
| @Parameter(name = ApiConstants.NAME, type = BaseCmd.CommandType.STRING, required = false, |
There was a problem hiding this comment.
name is marked required = false, but the manager uses cmd.getName() for uniqueness checks and to persist the cloned offering. If name is omitted, this can lead to NPEs/invalid DB rows. Make the parameter required (or implement a safe default like <sourceName>-clone).
| @Parameter(name = ApiConstants.NAME, type = BaseCmd.CommandType.STRING, required = false, | |
| @Parameter(name = ApiConstants.NAME, type = BaseCmd.CommandType.STRING, required = true, |
| } | ||
| normalizedDropServices.add(service.getName()); | ||
| } | ||
| finalServices.removeAll(dropServices); |
There was a problem hiding this comment.
normalizedDropServices is computed to canonicalize service names, but the code removes dropServices (raw input) from finalServices instead of removing normalizedDropServices. This can fail to drop services when callers provide non-canonical casing/names, and it leaves normalizedDropServices unused. Use finalServices.removeAll(normalizedDropServices).
| finalServices.removeAll(dropServices); | |
| finalServices.removeAll(normalizedDropServices); |
| }).then(json => { | ||
| this.diskOfferings = json.listdiskofferingsresponse.diskoffering || [] | ||
| if (this.selectedDiskOfferingId === '') { | ||
| this.selectedDiskOfferingId = this.diskOfferings[0].id || '' | ||
| } |
There was a problem hiding this comment.
this.diskOfferings[0].id will throw if listDiskOfferings returns an empty list. Guard with a length check (or use optional chaining) before reading the first element.
| <a-switch v-model:checked="form.encryptdisk" :checked="encryptdisk" @change="val => { encryptdisk = val }" /> | ||
| </a-form-item> | ||
| <a-form-item name="disksizestrictness" ref="disksizestrictness"> | ||
| <template #label> | ||
| <tooltip-label :title="$t('label.disksizestrictness')" :tooltip="apiParams.disksizestrictness.description" /> | ||
| </template> | ||
| <a-switch v-model:checked="form.disksizestrictness" :checked="disksizestrictness" @change="val => { disksizestrictness = val }" /> |
There was a problem hiding this comment.
The switch uses both v-model:checked and an explicit :checked bound to a different data property (encryptdisk). This can desynchronize the UI from form.encryptdisk (especially when initialValues sets a non-default). Prefer binding only via v-model:checked and remove the separate :checked/@change state mirror.
| <a-switch v-model:checked="form.encryptdisk" :checked="encryptdisk" @change="val => { encryptdisk = val }" /> | |
| </a-form-item> | |
| <a-form-item name="disksizestrictness" ref="disksizestrictness"> | |
| <template #label> | |
| <tooltip-label :title="$t('label.disksizestrictness')" :tooltip="apiParams.disksizestrictness.description" /> | |
| </template> | |
| <a-switch v-model:checked="form.disksizestrictness" :checked="disksizestrictness" @change="val => { disksizestrictness = val }" /> | |
| <a-switch v-model:checked="form.encryptdisk" /> | |
| </a-form-item> | |
| <a-form-item name="disksizestrictness" ref="disksizestrictness"> | |
| <template #label> | |
| <tooltip-label :title="$t('label.disksizestrictness')" :tooltip="apiParams.disksizestrictness.description" /> | |
| </template> | |
| <a-switch v-model:checked="form.disksizestrictness" /> |
| <a-switch v-model:checked="form.encryptdisk" :checked="encryptdisk" @change="val => { encryptdisk = val }" /> | ||
| </a-form-item> | ||
| <a-form-item name="disksizestrictness" ref="disksizestrictness"> | ||
| <template #label> | ||
| <tooltip-label :title="$t('label.disksizestrictness')" :tooltip="apiParams.disksizestrictness.description" /> | ||
| </template> | ||
| <a-switch v-model:checked="form.disksizestrictness" :checked="disksizestrictness" @change="val => { disksizestrictness = val }" /> |
There was a problem hiding this comment.
Same issue as encrypt switch: v-model:checked plus explicit :checked backed by disksizestrictness can cause the displayed value to diverge from form.disksizestrictness / initialValues. Bind the switch to a single source of truth (the form model).
| <a-switch v-model:checked="form.encryptdisk" :checked="encryptdisk" @change="val => { encryptdisk = val }" /> | |
| </a-form-item> | |
| <a-form-item name="disksizestrictness" ref="disksizestrictness"> | |
| <template #label> | |
| <tooltip-label :title="$t('label.disksizestrictness')" :tooltip="apiParams.disksizestrictness.description" /> | |
| </template> | |
| <a-switch v-model:checked="form.disksizestrictness" :checked="disksizestrictness" @change="val => { disksizestrictness = val }" /> | |
| <a-switch v-model:checked="form.encryptdisk" /> | |
| </a-form-item> | |
| <a-form-item name="disksizestrictness" ref="disksizestrictness"> | |
| <template #label> | |
| <tooltip-label :title="$t('label.disksizestrictness')" :tooltip="apiParams.disksizestrictness.description" /> | |
| </template> | |
| <a-switch v-model:checked="form.disksizestrictness" /> |
| } | ||
| }, | ||
| handleWriteCacheTypeChange (val) { | ||
| this.form.writeCacheType = val |
There was a problem hiding this comment.
handleWriteCacheTypeChange writes to form.writeCacheType, but the form field is writecachetype (lowercase). As written, this handler doesn't update the value that gets validated/submitted. Either remove the handler (since v-model already updates form.writecachetype) or update the correct property name.
| this.form.writeCacheType = val | |
| this.form.writecachetype = val |
2 participants
Description
This PR adds APIs to clone existing offerings - service, system, network, VPC, backup and update certain parameters of the existing offering.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?