Skip to content
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

API to validate Quota activation rule #9605

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

hsato03
Copy link
Collaborator

@hsato03 hsato03 commented Aug 29, 2024

Description

When entering an activation rule during the creation and editing of Quota tariffs, the defined rule is not validated. This way, errors are only identified while processing the tariff.

Because of this, the quotaValidateActivationRule API was created, which informs whether the activation rule is valid or not. Also, the activation rule field was added when creating and editing the tariff via UI.

API parameters

Parameter Description Required
activationrule The activation rule that will be validated. Yes
quotaresourcetype tariff usage type to validate the variables used in the script. Yes

Example

🐱 > quota validateactivationrule activationrule="if (account.name == 'admin') { 0 } else { 300 }" usagetype=6
{
  "validactivationrule": {
    "activationrule": "if (account.name == 'admin') { 0 } else { 300 }",
    "isvalid": true,
    "message": "The script has no syntax errors and all variables are compatible with the given usage type.",
    "quotatype": "VOLUME"
  }
}

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • 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
  • Minor
  • Trivial

Screenshots (if appropriate):

invalid-activation-rule

valid-activation-rule

How Has This Been Tested?

Test Valid activation rule
1 Activation rule with valid JS code and with variables compatible with the given usage type Yes
2 Activation rule with valid JS code and with variables that are not compatible with the given usage type No
3 Activation rule with invalid JS code No

@hsato03
Copy link
Collaborator Author

hsato03 commented Aug 29, 2024

@blueorangutan package

@blueorangutan
Copy link

@hsato03 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.

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 78.29787% with 51 lines in your changes missing coverage. Please review.

Project coverage is 15.81%. Comparing base (04c428c) to head (83ad4d1).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
.../cloudstack/jsinterpreter/JsInterpreterHelper.java 89.51% 10 Missing and 3 partials ⚠️
...ck/api/command/QuotaValidateActivationRuleCmd.java 33.33% 12 Missing ⚠️
.../response/QuotaValidateActivationRuleResponse.java 62.96% 9 Missing and 1 partial ⚠️
...g/apache/cloudstack/quota/constant/QuotaTypes.java 0.00% 9 Missing ⚠️
...ctivationrule/presetvariables/PresetVariables.java 0.00% 6 Missing ⚠️
.../org/apache/cloudstack/quota/QuotaServiceImpl.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9605      +/-   ##
============================================
+ Coverage     15.78%   15.81%   +0.02%     
- Complexity    12551    12597      +46     
============================================
  Files          5625     5628       +3     
  Lines        491958   492225     +267     
  Branches      63091    61373    -1718     
============================================
+ Hits          77663    77847     +184     
- Misses       405836   405916      +80     
- Partials       8459     8462       +3     
Flag Coverage Δ
uitests 4.03% <ø> (-0.01%) ⬇️
unittests 16.63% <78.29%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10872

Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Copy link
Collaborator

@bernardodemarco bernardodemarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, manually tested in a local environment.

Test descriptions

Firstly, I validated whether the quotaValidateActivationRule API would correctly identify JavaScript syntax errors.

image

The API correctly recognized the syntax error, which is the expected behavior.


Next, I inserted a valid syntax, and no errors were returned in the API response.

image


Relating to the validations of the preset variables, I selected the Template usage type and inserted the value.computingResources.cpuNumber variable into the script. Since it is not compatible with the selected usage type, an error was returned.

image


Then, I changed the usage type to Running VM and the script had been successfully validated, since the variable value.computingResources.cpuNumber is compatible with the selected usage type.

image

Copy link
Contributor

@BryanMLima BryanMLima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor suggestions, overall, LGTM.

@BryanMLima
Copy link
Contributor

@hsato03, the build is failing due to the logger variable in the VmwareHelper.java class, it should be fixed by #9714.

@hsato03
Copy link
Collaborator Author

hsato03 commented Sep 27, 2024

@blueorangutan package

@blueorangutan
Copy link

@hsato03 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
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11224

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-11578)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 69038 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9605-t11578-kvm-ol8.zip
Smoke tests completed. 134 look OK, 1 have errors, 6 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
ContextSuite context=TestISOUsage>:setup Error 0.00 test_usage.py
all_test_vpc_router_nics Skipped --- test_vpc_router_nics.py
all_test_vpc_vpn Skipped --- test_vpc_vpn.py
all_test_webhook_delivery Skipped --- test_webhook_delivery.py
all_test_webhook_lifecycle Skipped --- test_webhook_lifecycle.py
all_test_host_maintenance Skipped --- test_host_maintenance.py
all_test_hostha_kvm Skipped --- test_hostha_kvm.py

@hsato03
Copy link
Collaborator Author

hsato03 commented Oct 2, 2024

@DaanHoogland the test error logs don't seem to be related to the PR.

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/nose/suite.py", line 210, in run
    self.setUp()
  File "/usr/local/lib/python3.6/site-packages/nose/suite.py", line 293, in setUp
    self.setupContext(ancestor)
  File "/usr/local/lib/python3.6/site-packages/nose/suite.py", line 316, in setupContext
    try_run(context, names)
  File "/usr/local/lib/python3.6/site-packages/nose/util.py", line 471, in try_run
    return func()
  File "/marvin/tests/smoke/test_usage.py", line 1025, in setUpClass
    cls.iso.id
Exception: ISO download failed exception: Failed to download ISO: 2785331c-21e7-47d5-91d5-ffb46b2dbe32

Could you run the smoke tests again?

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland 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
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11280

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-11617)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 59990 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9605-t11617-kvm-ol8.zip
Smoke tests completed. 139 look OK, 0 have errors, 2 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
all_test_vm_strict_host_tags Skipped --- test_vm_strict_host_tags.py
all_test_vnf_templates Skipped --- test_vnf_templates.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants