-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Refactor Quota Summary API #10505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Refactor Quota Summary API #10505
Conversation
|
@blueorangutan package |
|
@julien-vaz 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 ✖️ debian ✖️ suse15. SL-JID 12665 |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #10505 +/- ##
=============================================
- Coverage 16.15% 3.63% -12.53%
=============================================
Files 5666 441 -5225
Lines 498081 37019 -461062
Branches 60267 6785 -53482
=============================================
- Hits 80475 1345 -79130
+ Misses 408593 35513 -373080
+ Partials 9013 161 -8852
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:
|
|
@blueorangutan package |
|
@julien-vaz 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 ✖️ debian ✖️ suse15. SL-JID 12690 |
|
I've just successfully builded the packages locally with |
|
@blueorangutan package |
|
@julien-vaz 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 ✖️ debian ✖️ suse15. SL-JID 12739 |
|
@blueorangutan package |
|
@julien-vaz 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 ✔️ debian ✔️ suse15. SL-JID 12743 |
plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaSummaryCmd.java
Outdated
Show resolved
Hide resolved
plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaSummaryCmd.java
Outdated
Show resolved
Hide resolved
…/command/QuotaSummaryCmd.java Co-authored-by: Fabricio Duarte <[email protected]>
|
@blueorangutan package |
|
@sureshanaparti 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. |
|
I still need to work on |
|
@blueorangutan package |
|
@julien-vaz 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 ✔️ debian ✔️ suse15. SL-JID 14583 |
plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaSummaryCmd.java
Show resolved
Hide resolved
|
@blueorangutan package |
|
@julien-vaz 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 ✖️ debian ✖️ suse15. SL-JID 14755 |
|
|
@blueorangutan package |
|
@julien-vaz 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 ✖️ debian ✖️ suse15. SL-JID 14821 |
|
@julien-vaz There is still failure in the build, can you please fix it and suggest how far this PR is ready, has this been tested recently ? |
nvazquez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @julien-vaz, some comments from my review
| } | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly also add a StringUtils.isBlank(value) check and return null in that case before the for loop?
| sc.setParameters("id", parentId, childId); | ||
|
|
||
| List<DomainVO> domainPair = listBy(sc); | ||
| List<DomainVO> domainPair = listIncludingRemovedBy(sc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm not clear why should removed domains should be considered?
| QuotaService quotaService; | ||
|
|
||
| @ACL | ||
| @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "ID of the account for which balance will be listed. Can not be specified with projectId.", since = "4.21.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since parameter needs updating :)
| super(); | ||
| } | ||
| @ACL | ||
| @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "Project Id for which balance will be listed. Can not be specified with accountId.", since = "4.21.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here
| private Long projectId; | ||
|
|
||
| @Override | ||
| public void execute() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this method include some pre-checks for accountId vs projectId parameters?
| * Returns the Id of the account that will be used when provided with either accountId, projectId or accountName and domainId. | ||
| */ | ||
| @Override | ||
| public Long finalizeAccountId(Long accountId, String accountName, Long domainId, Long projectId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic could be extracted into submethods
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Description
The
quotaSummaryAPI has abstract behaviors and codes, as well as return values not aligned with its purposes. Furthermore, when the account is a project, no information is returned.Also, in the UI, when opening the Quota Summary details for accounts who owned removed domains, the CloudStack launches an error to the user.
Therefore, this PR:
listall=true);Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
On CloudMonkey I called
quotaSummaryAPI and checked the return value:On the UI, the filter for removed accounts is working properly.