Repository navigation
config cleanup - #13884
config cleanup#13884DaanHoogland wants to merge 50 commits into
Conversation
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 589303 42558 -546745
Branches 72245 8063 -64182
=============================================
- Hits 120525 1578 -118947
+ Misses 455643 40751 -414892
+ Partials 13135 229 -12906
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 | 24.69% |
| Branch coverage | 18.92% |
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
…nfig enum to services
…ecated Config enum to services
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@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 ✖️ debian ✖️ suse15. SL-JID 19628 |
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
5 open issues
The default value hard-codes credentials (\"admin1:AdMiN123\") for HAProxy stats auth. This is an…Long.parseLong(_configDao.getValue(...))can throw NPE/NumberFormatException when the config key… This introduces yet another localConfigKeydefinition fordetail.batch.query.size(similar… · New The description text should use the plural form for units. Consider changing 'In second' to 'In… · New The description forpublish.resource.state.eventsis inaccurate: it refers to 'alert events', but… · New
1 resolved since last review
Review effort: Lite
| ConfigKey<Integer> CopyVolumeWait = new ConfigKey<>("Storage", Integer.class, "copy.volume.wait", "10800", | ||
| "In second, timeout for copy volume command", true); | ||
|
|
||
| ConfigKey<Integer> CreateVolumeFromSnapshotWait = new ConfigKey<>("Storage", Integer.class, "create.volume.from.snapshot.wait", "10800", | ||
| "In second, timeout for creating volume from snapshot", 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
7 open issues
The default value hard-codes credentials (\"admin1:AdMiN123\") for HAProxy stats auth. This is an…Long.parseLong(_configDao.getValue(...))can throw NPE/NumberFormatException when the config key… The HAProxy stats port is modeled as aStringbut is consumed numerically elsewhere (e.g., parsed… · New This PR introduces multiple local duplicatedConfigKeydeclarations for the same underlying… · New This introduces yet another localConfigKeydefinition fordetail.batch.query.size(similar… The description forpublish.resource.state.eventsis inaccurate: it refers to 'alert events', but… The description text should use the plural form for units. Consider changing 'In second' to 'In…
Review effort: Lite
| ConfigKey<String> NetworkLBHaproxyStatsPort = new ConfigKey<>("Network", String.class, | ||
| "network.loadbalancer.haproxy.stats.port", "8081", | ||
| "Load Balancer(haproxy) stats port number.", true); |
| // Registered/owned by ManagementServer; declared here too so this lower-layer module can read it without depending on the server module. | ||
| private static final ConfigKey<Integer> DetailBatchQuerySize = new ConfigKey<>("Advanced", Integer.class, "detail.batch.query.size", "2000", | ||
| "Default entity detail batch query size for listing", true); |



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?