chore: add support for system label application rules #235
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #235 +/- ##
============================================
- Coverage 79.93% 78.92% -1.01%
- Complexity 496 524 +28
============================================
Files 55 57 +2
Lines 2432 2567 +135
Branches 108 120 +12
============================================
+ Hits 1944 2026 +82
- Misses 427 471 +44
- Partials 61 70 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
.../hypertrace/label/application/rule/config/service/LabelApplicationRuleConfigServiceImpl.java
Outdated
Show resolved
Hide resolved
| labelApplicationRuleConfig.getInt(MAX_DYNAMIC_LABEL_APPLICATION_RULES_PER_TENANT); | ||
| } | ||
| Config labelApplicationRuleConfig = | ||
| config.hasPath(LABEL_APPLICATION_RULE_CONFIG_SERVICE_CONFIG) |
There was a problem hiding this comment.
hasPath checks aren't necessary - just put an empty array as a default value in application.conf (which is less code and makes the config easier to understand anyway). Same goes for the other configs, although they're pre-existing.
There was a problem hiding this comment.
Keeping it as is. May require changes across multiple application.conf
There was a problem hiding this comment.
Separate PR is fine, but please come back to this. It will only require a change in the default application conf to explicitly declare the default values rather than having the defaults defined across code, conf, and helm.
| this.requestValidator.validateOrThrow(requestContext, request); | ||
| String labelApplicationRuleId = request.getId(); | ||
| if (systemLabelApplicationRulesMap.containsKey(labelApplicationRuleId)) { | ||
| // Deleting a system label application rule is not allowed |
There was a problem hiding this comment.
We generally allow users to delete default config. Why is this different?
There was a problem hiding this comment.
Can be done. However deleting a default rule will require a new config store to persist it. Will add it if need arises.
There was a problem hiding this comment.
will do it in separate PR
.../hypertrace/label/application/rule/config/service/LabelApplicationRuleConfigServiceImpl.java
Outdated
Show resolved
Hide resolved
...ertrace/label/application/rule/config/service/LabelApplicationRuleConfigServiceImplTest.java
Outdated
Show resolved
Hide resolved
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class LabelApplicationRuleConfigServiceImplTest { | ||
| private static final String SYSTEM_LABEL_APPLICATION_RULE_STR = |
There was a problem hiding this comment.
This test is mocking the config, so mock it (i.e. return a prebuilt rule) rather than do the real parse logic and put it behind a mock. Not only is that not the responsibility of this class/test, but if a change were added to the rule structure this test would miss it silently.
There was a problem hiding this comment.
Done. Hopefully last review comment to address ;-)
No description provided.