Repository navigation
config cleanup - #13884
config cleanup#13884DaanHoogland wants to merge 52 commits into
Conversation
boring-cyborg
Bot
added
component:api
component:kvm
component:management-server
component:networking
component:orchestration
component:primary-storage
component:projects
component:quota
component:secondary-storage
component:security-group
component:ssvm
component:storage
component:usage-server
labels
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #13884 +/- ##
=============================================
- Coverage 20.45% 3.70% -16.75%
=============================================
Files 6487 493 -5994
Lines 589329 42558 -546771
Branches 72246 8063 -64183
=============================================
- Hits 120554 1578 -118976
+ Misses 455642 40751 -414891
+ Partials 13133 229 -12904
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.51% |
| Branch coverage | 19.83% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
11 open issues
configDepotis changed from a mockedConfigurationDaoto a plainConfigDepotImplfield, but… · NewconfigDepotis changed from a mockedConfigurationDaoto a plainConfigDepotImplfield, but… · NewLong.parseLong(_configDao.getValue(...))can throw NPE/NumberFormatException when the config key…ConfigKeys appear to change behavior vs the prior… · NewConfigKeydeclarations for the same underlying…Stringbut is consumed numerically elsewhere (e.g., parsed…ConfigKeydefinition fordetail.batch.query.size(similar…publish.resource.state.eventssays it controls publishing of 'alert events',… · Newpublish.resource.state.eventsis inaccurate: it refers to 'alert events', but…
Review effort: Lite
| "network.securitygroups.work.cleanup.interval", "120", | ||
| "Time interval (seconds) in which finished work is cleaned up from the work table", true); | ||
|
|
||
| ConfigKey<Integer> SecurityGroupWorkerThreads = new ConfigKey<>("Network", Integer.class, | ||
| "network.securitygroups.workers.pool.size", "50", |
| // Registered/owned by com.cloud.server.ManagementServer#PublishResourceStateEvent; declared here too so this | ||
| // module can read it without a reverse dependency on the server module. | ||
| private static final ConfigKey<Boolean> PublishResourceStateEvent = new ConfigKey<>("Advanced", Boolean.class, "publish.resource.state.events", "true", | ||
| "enable or disable publishing of alert events on the event bus", true); |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
14 open issues
configDepotis no longer annotated as a Mockito mock in the diff (it’s declared as a field but… · NewconfigDepotis no longer annotated as a Mockito mock in the diff (it’s declared as a field but… · NewconfigDepotis changed from a mockedConfigurationDaoto a plainConfigDepotImplfield, but…configDepotis changed from a mockedConfigurationDaoto a plainConfigDepotImplfield, but…Long.parseLong(_configDao.getValue(...))can throw NPE/NumberFormatException when the config key…ConfigKeys appear to change behavior vs the prior…ConfigKeydeclarations for the same underlying…Stringbut is consumed numerically elsewhere (e.g., parsed…ConfigKeydefinition fordetail.batch.query.size(similar…PublishResourceStateEventdescription says it controls publishing of \"alert events\", but… · Newpublish.resource.state.eventssays it controls publishing of 'alert events',…publish.resource.state.eventsis inaccurate: it refers to 'alert events', but…
Review effort: Lite
| @Mock | ||
| ConfigurationDao _configDao; | ||
| ConfigDepotImpl configDepot; |
| @Before | ||
| public void setup() { | ||
| when(_configDao.getValue(batchSizeConfigurationKey)).thenReturn(batchSizeValue); | ||
| ConfigKey.init(configDepot); | ||
| when(configDepot.getConfigStringValue(batchSizeConfigurationKey, ConfigKey.Scope.Global, null)).thenReturn(batchSizeValue); |
| // Registered/owned by com.cloud.server.ManagementServer#PublishResourceStateEvent; declared here too so this | ||
| // module can read it without a reverse dependency on the server module. | ||
| private static final ConfigKey<Boolean> PublishResourceStateEvent = new ConfigKey<>("Advanced", Boolean.class, "publish.resource.state.events", "true", | ||
| "enable or disable publishing of alert events on the event bus", true); |
|
@nvazquez , as discussed I am busy splitting this PR with the help of AI. I will close this one as soon as I am satisfied the cascade of PRs, that I'll be creating cover everything. I will keep this open and mark as draft for testing purposes. |
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19670 |
|
This PR has been split into smaller PRs: Foundation (merge first)
Parallel (based on the foundation; can be reviewed and merged in any order)
Stacked (merge in this order, starting after the foundation)
Last: removing the dead |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19713 |
|
@blueorangutan test matrix |
|
@DaanHoogland a [SL] Trillian-Jenkins matrix job (ol8 mgmt + kvm-ol8, ubuntu24 mgmt + kvm-ubuntu24, ol8 mgmt + vmware-70u3, ol9 mgmt + xcpng82) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-17252)
|
wow, nice |
|
[SF] Trillian test result (tid-17251)
|
|
[SF] Trillian test result (tid-17250)
|
|
[SF] Trillian test result (tid-17253)
|
Description
This PR tries to get rid of the Config enum or the old mechanism to define configuration items, which is long overdue. The change should be transparent, but is huge so good regression testing is required. And merg early in the release cycle is recommended.
Commits are being done in batches for review convenience. They could have been done in separate PRs but in the end the changes are rather trivial.
Fixes: #10752
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?