Improve Quota Statement #10506
Improve Quota Statement #10506
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 12669 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10506 +/- ##
============================================
+ Coverage 16.15% 16.58% +0.43%
- Complexity 13273 13883 +610
============================================
Files 5666 5725 +59
Lines 498081 507484 +9403
Branches 60267 61596 +1329
============================================
+ Hits 80475 84184 +3709
- Misses 408593 413883 +5290
- Partials 9013 9417 +404
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 12689 |
…uotaServiceImplTest
|
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 12742 |
plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaStatementCmd.java
Outdated
Show resolved
Hide resolved
...atabase/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java
Outdated
Show resolved
Hide resolved
| "accountName", "accountId", "domainId", "startDate", "endDate", "type", "showDetails"))); | ||
| } | ||
| logger.debug("Creating quota statement from [{}] usage records for parameters [{}].", quotaUsages.size(), | ||
| ReflectionToStringBuilderUtils.reflectOnlySelectedFields(cmd, "accountName", "accountId", "domainId", "startDate", "endDate", "type", "showDetails")); |
There was a problem hiding this comment.
this seems the reuse of the same array of strings. Not for this PR but maybe we should allow String[] as parameter for ReflectionToStringBuilderUtils.reflectOnlySelectedFields. Or allow some kind of predefined default per class to be registered.
...atabase/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java
Outdated
Show resolved
Hide resolved
|
@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. |
|
@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 13917 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
@DaanHoogland could we try the tests again? |
|
@blueorangutan test |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13690)
|
|
@blueorangutan package |
|
@harikrishna-patnala 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 15133 |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the quota statement API by addressing UUID usage, fixing dummy record bugs, and adding new functionality for resource detail display. The changes improve API consistency by returning UUIDs instead of internal IDs, resolve issues with dummy records when usage_type is specified, and introduce a new showresources parameter for detailed resource information.
- Return UUIDs instead of internal IDs for accounts and domains in quota statements
- Fix dummy record generation bug when usage_type parameter is provided
- Add new showresources parameter and quotaStatementDetails API for enhanced resource visibility
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| QuotaServiceImpl.java | Updates to use QuotaUsageJoinDao instead of QuotaUsageDao |
| QuotaResponseBuilderImpl.java | Major refactoring to handle new UUID responses and resource details |
| QuotaStatementResponse.java | Changes account/domain ID fields from Long to String for UUID support |
| QuotaStatementItemResponse.java | Removes duplicate account/domain fields and adds resources field |
| QuotaStatementCmd.java | Adds showresources parameter and simplifies date handling |
| Test files | Updates to accommodate new DAO and method signature changes |
| New VO classes | Addition of join and resource VOs to support enhanced functionality |
| New DAO classes | Implementation of join and detail DAOs for improved data access |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } catch (ParseException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Using e.printStackTrace() in test code is not recommended. Consider using a proper logging mechanism or re-throwing the exception as a RuntimeException to fail the test properly.
| } catch (ParseException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Using e.printStackTrace() in test code is not recommended. Consider using a proper logging mechanism or re-throwing the exception as a RuntimeException to fail the test properly.
| } catch (ParseException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Using e.printStackTrace() in test code is not recommended. Consider using a proper logging mechanism or re-throwing the exception as a RuntimeException to fail the test properly.
| public void setStartDate(Date startDate) { this.startDate = startDate; } | ||
|
|
||
| public boolean isShowResources() { return showResources; } | ||
|
|
||
| public void setShowResources(boolean showResources) { this.showResources = showResources; } |
There was a problem hiding this comment.
Method declarations should be formatted consistently with proper spacing. Each method should be on separate lines with proper indentation for better readability.
| public void setStartDate(Date startDate) { this.startDate = startDate; } | |
| public boolean isShowResources() { return showResources; } | |
| public void setShowResources(boolean showResources) { this.showResources = showResources; } | |
| public void setStartDate(Date startDate) { | |
| this.startDate = startDate; | |
| } | |
| public boolean isShowResources() { | |
| return showResources; | |
| } | |
| public void setShowResources(boolean showResources) { | |
| this.showResources = showResources; | |
| } |
| public void setStartDate(Date startDate) { this.startDate = startDate; } | ||
|
|
||
| public boolean isShowResources() { return showResources; } | ||
|
|
||
| public void setShowResources(boolean showResources) { this.showResources = showResources; } |
There was a problem hiding this comment.
Method declarations should be formatted consistently with proper spacing. Each method should be on separate lines with proper indentation for better readability.
| public void setStartDate(Date startDate) { this.startDate = startDate; } | |
| public boolean isShowResources() { return showResources; } | |
| public void setShowResources(boolean showResources) { this.showResources = showResources; } | |
| public void setStartDate(Date startDate) { | |
| this.startDate = startDate; | |
| } | |
| public boolean isShowResources() { | |
| return showResources; | |
| } | |
| public void setShowResources(boolean showResources) { | |
| this.showResources = showResources; | |
| } |
| public void setStartDate(Date startDate) { this.startDate = startDate; } | ||
|
|
||
| public boolean isShowResources() { return showResources; } | ||
|
|
||
| public void setShowResources(boolean showResources) { this.showResources = showResources; } |
There was a problem hiding this comment.
Method declarations should be formatted consistently with proper spacing. Each method should be on separate lines with proper indentation for better readability.
| public void setStartDate(Date startDate) { this.startDate = startDate; } | |
| public boolean isShowResources() { return showResources; } | |
| public void setShowResources(boolean showResources) { this.showResources = showResources; } | |
| public void setStartDate(Date startDate) { | |
| this.startDate = startDate; | |
| } | |
| public boolean isShowResources() { | |
| return showResources; | |
| } | |
| public void setShowResources(boolean showResources) { | |
| this.showResources = showResources; | |
| } |
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
Are schema changes missing here? Seeing some new VO classes but no corresponding SQL changes I can find
|
|
||
| List<AccountVO> listAccounts(String accountName, Long domainId, Filter filter); | ||
|
|
||
| AccountVO findAccountByNameAndDomainIncludingRemoved(String accountName, Long domainId); |
There was a problem hiding this comment.
same as findAccountIncludingRemoved
| import org.apache.commons.lang3.builder.ToStringStyle; | ||
|
|
||
| @Entity | ||
| @Table(name = "quota_usage_detail") |
There was a problem hiding this comment.
I feel table should be named differently as CLoudStack uses *_detail table for the details of the entity mostly in the similar format resource_id,name,value
|
|
||
| import java.util.Date; | ||
|
|
||
| public class QuotaUsageResourceVO { |
There was a problem hiding this comment.
Is this referring to any DB table?
|
Hi @julien-vaz is this PR ready for review? Looks like its missing SQL statements for new tables and views? |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
12 participants
Description
In the current version, the
quotaStatementAPI is returning the internal ID for account and domain, which is not useful for users, since all APIs use/return the UUID. Also, when theusage_typeparameter is informed, the API shows dummy records.To address those problems:
showresourceswas added to display more information to the user about each usage type;quotaStatementDetailsAPI was created to list more details about each usage type;Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
On CloudMonkey the
quotaStatementAPI was called and:usage_typeno dummy records were showed;showresourcesparameter is properly working;The
quotaStatementDetailsAPI was called successfully as well.