Skip to content

remove supportedOwner from Resource.ResourceType#7416

Merged
weizhouapache merged 3 commits intoapache:mainfrom
scclouds:remove_supportsOwner_from_ResourceType
Aug 30, 2023
Merged

remove supportedOwner from Resource.ResourceType#7416
weizhouapache merged 3 commits intoapache:mainfrom
scclouds:remove_supportsOwner_from_ResourceType

Conversation

@GaOrtiga
Copy link
Contributor

@GaOrtiga GaOrtiga commented Apr 6, 2023

Description

The ResourceType enum, which is part of the Resource interface, includes an argument called supportedOwners that is used to indicate which types of owners are supported - either Domain, Account or both. However, every type of owner enumerated is supported by both options, making this validation unnecessary.

This argument has been removed, along with its getter method.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 6, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #7416 (59dab74) into main (729e6d1) will decrease coverage by 0.73%.
Report is 126 commits behind head on main.
The diff coverage is 0.00%.

❗ Current head 59dab74 differs from pull request most recent head e21df9f. Consider uploading reports for the commit e21df9f to get more accurate results

@@             Coverage Diff              @@
##               main    #7416      +/-   ##
============================================
- Coverage     13.42%   12.69%   -0.73%     
+ Complexity     9381     8664     -717     
============================================
  Files          2747     2718      -29     
  Lines        258798   256332    -2466     
  Branches      40312    39954     -358     
============================================
- Hits          34732    32539    -2193     
+ Misses       219695   219657      -38     
+ Partials       4371     4136     -235     
Files Changed Coverage Δ
.../cloud/configuration/dao/ResourceCountDaoImpl.java 9.43% <ø> (+0.58%) ⬆️
.../cloud/resourcelimit/ResourceLimitManagerImpl.java 1.09% <0.00%> (+0.06%) ⬆️
...java/com/cloud/server/ConfigurationServerImpl.java 2.32% <0.00%> (+0.02%) ⬆️

... and 246 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@GaOrtiga GaOrtiga marked this pull request as ready for review April 19, 2023 10:48
@GaOrtiga
Copy link
Contributor Author

GaOrtiga commented May 4, 2023

Thanks for reviewing @DaanHoogland.

Could anyone else review this one?

@DaanHoogland DaanHoogland added this to the 4.19.0.0 milestone Jun 22, 2023
@GaOrtiga GaOrtiga requested a review from yadvr June 26, 2023 11:13
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

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

@GaOrtiga
Copy link
Contributor Author

@GutoVeronezi @weizhouapache
Could you guys review this one?

Copy link
Contributor

@GutoVeronezi GutoVeronezi left a comment

Choose a reason for hiding this comment

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

CLGTM

Thansk @GaOrtiga

@weizhouapache
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@weizhouapache a [SF] 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]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6920

@weizhouapache
Copy link
Member

@blueorangutan test

@blueorangutan
Copy link

@weizhouapache a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-7592)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37247 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7416-t7592-kvm-centos7.zip
Smoke tests completed. 110 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
ContextSuite context=TestVmAutoScaling>:setup Error 0.00 test_vm_autoscaling.py
test_05_vmschedule_test_e2e Failure 270.49 test_vm_schedule.py
test_02_cancel_host_maintenace_with_migration_jobs Error 0.40 test_host_maintenance.py
test_03_cancel_host_maintenace_with_migration_jobs_failure Error 0.54 test_host_maintenance.py
test_01_cancel_host_maintenance_ssh_enabled_agent_connected Failure 53.70 test_host_maintenance.py
test_03_cancel_host_maintenance_ssh_disabled_agent_connected Failure 20.46 test_host_maintenance.py
test_04_cancel_host_maintenance_ssh_disabled_agent_disconnected Failure 31.43 test_host_maintenance.py

@GaOrtiga
Copy link
Contributor Author

I checked the tests and they are not related to my PR; they are failing in other PRs as well, like in this and this. @weizhouapache are we good to go?

@weizhouapache
Copy link
Member

I checked the tests and they are not related to my PR; they are failing in other PRs as well, like in this and this. @weizhouapache are we good to go?

@GaOrtiga
code lgtm

However, I do not know why the original author added the code (>10 years ago), and if it will be used in the future.
I will leave to the community to determine if they can be removed.

@GaOrtiga
Copy link
Contributor Author

Thanks for the review @weizhouapache

However, I do not know why the original author added the code (>10 years ago), and if it will be used in the future. I will leave to the community to determine if they can be removed.

I do not know the original author's intentions either. However, it has never been used and there are no immediate plans to use it, so as of now it is useless code. In my opinion this should be removed, and, if at some point in the future a feature is implemented which uses these parameters, they can just be added back in.

@DaanHoogland
Copy link
Contributor

@weizhouapache (cc @GaOrtiga ) I think that this code can be removed. Nobody is using it and we have a version control system so we can always put it back.

@weizhouapache
Copy link
Member

@weizhouapache (cc @GaOrtiga ) I think that this code can be removed. Nobody is using it and we have a version control system so we can always put it back.

ok, fair @DaanHoogland

Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

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

code lgtm

@weizhouapache weizhouapache merged commit 819dd7b into apache:main Aug 30, 2023
yadvr pushed a commit to shapeblue/cloudstack that referenced this pull request Apr 24, 2024
* Speed up resource count calculation

* server: remove supportedOwner from Resource.ResourceType (apache#7416)

* Refactor resource count calculation

* Start transaction for updateCountByDeltaForIds

---------

Co-authored-by: GaOrtiga <49285692+GaOrtiga@users.noreply.github.com>
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.

6 participants