Repository navigation
validates the inputs for capacity bytes in update storage pool to be positive - #14219
Open
sathvikaragi wants to merge 3 commits into
Open
sathvikaragi wants to merge 3 commits into
sathvikaragi wants to merge 3 commits into
Conversation
…ge pool to be positive. (#101) …r now validates inputs for positive number ### Description validates the inputs for capacity bytes in update storage pool to be positive. This PR... <!--- Describe your changes in DETAIL - And how has behaviour functionally changed. --> <!-- For new features, provide link to FS, dev ML discussion etc. --> <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. --> <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged --> <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" --> <!-- Fixes: # --> <!--- ******************************************************************************* --> <!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. --> <!--- PLEASE PUT AN 'X' in only **ONE** box --> <!--- ******************************************************************************* --> ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [X] Bug fix (non-breaking change which fixes an issue) - [ ] Enhancement (improves an existing feature and functionality) - [ ] Cleanup (Code refactoring and cleanup, that may add test cases) - [ ] Build/CI - [ ] Test (unit or integration test code) ### Feature/Enhancement Scale or Bug Severity #### Feature/Enhancement Scale - [ ] Major - [ ] Minor #### Bug Severity - [ ] BLOCKER - [ ] Critical - [ ] Major - [X] Minor - [ ] Trivial ### Screenshots (if appropriate): ### How Has This Been Tested? <!-- Please describe in detail how you tested your changes. --> <!-- Include details of your testing environment, and the tests you ran to --> #### How did you try to break this feature and the system with this change? <!-- see how your change affects other areas of the code, etc. --> <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
sathvikaragi
marked this pull request as ready for review
September 22, 2026 06:26
Author
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14219 +/- ##
============================================
- Coverage 19.91% 19.90% -0.02%
+ Complexity 20200 20194 -6
============================================
Files 6373 6373
Lines 577230 577234 +4
Branches 70696 70696
============================================
- Hits 114974 114905 -69
- Misses 449690 449773 +83
+ Partials 12566 12556 -10
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:
|
Contributor
|
@blueorangutan package |
Contributor
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 19438 |
Damans227
reviewed
Oct 7, 2026
| @@ -56,7 +56,7 @@ public class UpdateStoragePoolCmd extends BaseCmd { | |||
| @Parameter(name = ApiConstants.CAPACITY_IOPS, type = CommandType.LONG, required = false, description = "IOPS CloudStack can provision from this storage pool") | |||
Collaborator
There was a problem hiding this comment.
should capacityiops get the same check? a negative value still goes through on that one
Author
There was a problem hiding this comment.
Good catch! We already have this change done as a part of another PR which is in pipeline, but we can put it in this PR.
Collaborator
There was a problem hiding this comment.
sounds good, putting it here keeps both checks together. ill look again once its pushed
Damans227
reviewed
Oct 7, 2026
| private Long capacityIops; | ||
|
|
||
| @Parameter(name = ApiConstants.CAPACITY_BYTES, type = CommandType.LONG, required = false, description = "Bytes CloudStack can provision from this storage pool") | ||
| @Parameter(name = ApiConstants.CAPACITY_BYTES, type = CommandType.LONG, required = false, description = "Bytes CloudStack can provision from this storage pool",validations={ApiArgValidator.PositiveNumber}) |
Collaborator
There was a problem hiding this comment.
can we add the same check on createStoragePool too? it still takes a negative size when the pool is first added
Author
There was a problem hiding this comment.
Sure, I agree API-level validation should be consistent, so I’ll add the same validation to createStoragePool as well.
Collaborator
There was a problem hiding this comment.
makes sense, thanks. ill check it with the next push
#116) …n createstoragepoolCmd.java ### Description Capacitybytes and capacityIOPS parameter in create storage pool has to have enforcement of positive number using APIArgValidator and fail fast. This PR... <!--- Describe your changes in DETAIL - And how has behaviour functionally changed. --> <!-- For new features, provide link to FS, dev ML discussion etc. --> <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. --> <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged --> <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" --> <!-- Fixes: # --> <!--- ******************************************************************************* --> <!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. --> <!--- PLEASE PUT AN 'X' in only **ONE** box --> <!--- ******************************************************************************* --> ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [X] Bug fix (non-breaking change which fixes an issue) - [ ] Enhancement (improves an existing feature and functionality) - [ ] Cleanup (Code refactoring and cleanup, that may add test cases) - [ ] Build/CI - [ ] Test (unit or integration test code) ### Feature/Enhancement Scale or Bug Severity #### Feature/Enhancement Scale - [ ] Major - [ ] Minor #### Bug Severity - [ ] BLOCKER - [ ] Critical - [ ] Major - [X] Minor - [ ] Trivial ### Screenshots (if appropriate): ### How Has This Been Tested? Before the fix : propagates the error that is given after hitting Ontap lifecycle: <img width="1721" height="1045" alt="Screenshot 2026-10-08 at 5 26 21 PM" src="https://github.com/user-attachments/assets/eefb609a-b2bb-456f-b4f3-b5140b847ecf" /> After the fix : <img width="1723" height="982" alt="Screenshot 2026-10-08 at 5 37 33 PM" src="https://github.com/user-attachments/assets/acfa1f96-17e8-4ff6-b689-48d1cbd4273e" /> <!-- Please describe in detail how you tested your changes. --> <!-- Include details of your testing environment, and the tests you ran to --> #### How did you try to break this feature and the system with this change? <!-- see how your change affects other areas of the code, etc. --> <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
Collaborator
|
thanks @sathvikaragi, fix looks right. LGTM. |
Author
|
@Damans227 Thankyou so much for the quick response, Please approve the PR! |
Damans227
approved these changes
Oct 8, 2026
Damans227
left a comment
Damans227
left a comment
Collaborator
There was a problem hiding this comment.
approving without a lab test since this reuses the same above-zero check many other commands already have, and your screenshots show it rejecting a negative value.
DaanHoogland
approved these changes
Oct 12, 2026
DaanHoogland
left a comment
DaanHoogland
left a comment
Contributor
There was a problem hiding this comment.
clgtm, agree with @Damans227 that we can rely on gha tests for this one.
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
4 participants
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
validates the inputs for capacity bytes in update storage pool to be positive. using the APIArgValidator
This PR...
Fixes: #14221
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?
Before the fix:
How did you try to break this feature and the system with this change?