Repository navigation
Conversation
clusterid is inherited from importUnmanagedInstance as a required parameter, but importVm with importsource external, local or shared never reads it: the deployment plan for disk imports is derived from the storage pool (and host for local storage). Make it optional at the API layer. The VMware and unmanaged import paths still reject a missing cluster in basicAccessChecks, now with a message naming the parameter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14435 +/- ##
=========================================
Coverage 18.03% 18.03%
- Complexity 16294 16295 +1
=========================================
Files 5937 5937
Lines 536832 536832
Branches 65793 65793
=========================================
+ Hits 96802 96812 +10
+ Misses 428977 428966 -11
- Partials 11053 11054 +1
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 branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR makes the
clusteridparameter of theimportVmAPI optional.ImportVmCmdextendsImportUnmanagedInstanceCmdand inheritsclusteridwithrequired = true, so everyimportVmcall must pass a valid cluster UUID. However, forimportsource=external,localandshared(importKvmInstance()), the value is never read:local/shareddisk imports, the deployment plan is built from the storage pool's pod and cluster (andhostidfor local storage), not fromclusterid.external, the instance is fetched from the remote host given byhost.Only
importsource=vmwareandunmanaged(baseImportInstance()) use the cluster. Those paths already reject a missing cluster inbasicAccessChecks(). This PR keeps that check and changes its message to name theclusteridparameter.importUnmanagedInstanceshares the same parameter field, so the API doc now listsclusteridas optional there too. The server still rejects a call without it, with anInvalidParameterValueExceptioninstead of the generic missing-parameter error, and the parameter description says when it is required.Before this change, for example:
After this change the call succeeds without
clusterid. Requests that pass it are unaffected, so the UI (which always sends it) keeps working.Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
How Has This Been Tested?
Unit tests in
UnmanagedVMsManagerImplTest(95 tests, all passing):importFromLocalDisk/importFromsharedStoragenow also check thatgetClusterId()is never called on the import path.testImportVmFromVmwareMissingClusterIdandtestImportVmFromUnmanagedMissingClusterIdcheck that VMware and unmanaged imports withoutclusteridstill fail withInvalidParameterValueException.How did you try to break this feature and the system with this change?
Checked every caller of
getClusterId()on the import commands: onlybaseImportInstance()(VMware/unmanaged) andlistUnmanagedInstancesread it, and both already check for null.🤖 Generated with Claude Code