-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Create API to list Quota credits #9590
base: main
Are you sure you want to change the base?
Conversation
@blueorangutan package |
@winterhazel 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9590 +/- ##
============================================
+ Coverage 15.54% 15.59% +0.04%
- Complexity 11996 12058 +62
============================================
Files 5499 5502 +3
Lines 481706 482180 +474
Branches 62623 62014 -609
============================================
+ Hits 74884 75185 +301
- Misses 398536 398691 +155
- Partials 8286 8304 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10810 |
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.
LGTM, did not test it
@blueorangutan test keepEnv |
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
[SF] Trillian test result (tid-11210)
|
@winterhazel , I think this is good to go safe one concern; removing response data from an API response is a backwards incompatibility. I can not judge the gravity (and haven't tested this PR and am not a quota user so just take my concern a little seriously ;) but the principle should only be broken after careful consideration. Can you give some stronger arguments than consistency for removing it, please? |
Hey @DaanHoogland, my idea of removing the credits listing from Regarding the gravity of removing it: Quota users would be advised to use As it would definitely have an impact, I understand why we would want to keep this information in the response, so I'm okay with not removing it. The enhancements to |
ok, @winterhazel . I know you and your are the most heavy users of quota so I am not going to -1 this by any means, but my advice would be to leave it in and shout about removing it before you do. you might get work mitigating any regressions if you don't. |
Description
When credits are added to an account, an entry that represents the operation is created in
cloud_usage.quota_credits
. Some information about these entries is currently returned alongside an account's balance through thequotaBalance
API. However, this is not consistent with the API's purpose (viewing an account's balance), and it is not possible to view the credit addition history for a whole domain with a single API call.In order to make
quotaBalance
more consistent with its purpose, the credit addition history will be removed from its response alongside some enhancements to the API in the future. To list the credit addition history, this PR adds thequotaCreditsList
, which has the following parameters:accountid
: ID of the account for which the credit statement will be generated.domainid
: ID of the domain for which credit statement will be generated. Available only for administrators.isrecursive
: Whether to generate the credit statement for the provided domain and its children. Defaults to false.startdate
: Start date of the statement. If not provided, the first day of the current month will be considered as the start date.enddate
: End date of the statement. If not provided, the current date will be considered as the end date.No parameters are flagged as required, but at least
accountid
ordomainid
must be provided.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
In my environment, there were the following domains and accounts:
ROOT
: admin (root admin)ROOT/d1
: d1 (domain admin), u1 (user)First, in the root admin account, I added 100 credits to the three accounts; in the domain admin account, I added credits to d1 and u1. Then, I tested the behavior of the API for each account.
For the root admin account:
accountid
tested).enddate
tested).startdate
tested).ROOT
's credits and verified that the response had the expected results (domainid
tested).ROOT
's credits recursively and verified that the response had the expected results (isrecursive
tested).For the domain admin account:
ROOT/d1
's credits history and verified that the response had the expected results.For the user account:
ROOT/d1
and verified that my permission was denied.