Allow user to update system labels #270
Conversation
Vaibhav090420
requested review from
aman-bansal,
aaron-steinfeld,
skjindal93 and
suresh-prakash
Vaibhav090420
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #270 +/- ##
============================================
- Coverage 79.93% 77.24% -2.70%
- Complexity 496 548 +52
============================================
Files 55 60 +5
Lines 2432 2689 +257
Branches 108 122 +14
============================================
+ Hits 1944 2077 +133
- Misses 427 540 +113
- Partials 61 72 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| () -> | ||
| Optional.ofNullable( | ||
| systemLabelInfoUtils | ||
| .getSystemLabelsIdLabelMap(getExistingLabels(requestContext)) |
There was a problem hiding this comment.
i am confused here? why do you need to find system labels ids from existing labels? We dont change system config ids when we store them in DB
There was a problem hiding this comment.
It should be as simple as
Label label =
labelStore
.getData(requestContext, labelId)
.orElseGet(
() ->
Optional.ofNullable(
systemLabelInfoUtils
.getDefaultSystemLabels()
.filter(label -> label.getId().equals(labelId))
.findFirst()
.orElseThrow(Status.NOT_FOUND::asRuntimeException));
| this.config = config; | ||
| } | ||
|
|
||
| public List<Label> getSystemLabels(List<Label> existingLabels) { |
There was a problem hiding this comment.
i dont think you would ever need to evaluate system labels from existing labels. same comment for all methods
There was a problem hiding this comment.
In case of system label name updation, we would require it to evaluate from the existing labels in db. If we do not want the name updation on system label then, it is not required
| @@ -194,15 +156,16 @@ public void getLabel(GetLabelRequest request, StreamObserver<GetLabelResponse> r | |||
| RequestContext requestContext = RequestContext.CURRENT.get(); | |||
| String labelId = request.getId(); | |||
| try { | |||
| Label label; | |||
There was a problem hiding this comment.
You would need to make changes in almost all the methods available in this api. So the flow would be like this
- We will build the defaultLabels list from the config.
- Now any update request can come from Clients. If update is for a label with id from system labels, then you save the entry in Db after doing necessary validation. For example we might not want to update names for system labels. Add necessary validation wherever needed.
- In case of read, We need to merge DB based labels and system labels while giving priority to DB entry and reorder if necessary. For example we always generally wants system label to be at the end of the list
Vaibhav090420
force-pushed
the
allow-label-updation
branch
from
d96c04d to
071d313
Compare
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.