-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: add new label package #18078
Conversation
8d9f68a
to
9eafd46
Compare
96b9fa7
to
f0e9840
Compare
f0e9840
to
4778612
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is awesome 🎉 Thanks for the helpful description as well and the comment on orgID. That got my mental cogs turning :D
I mention a couple of things which I think need addressing. I see we used to use an org service to do a find org for resource
lookup, in order to authorize. This is hard to maintain and I also want to see it go. I am not certain the orgID threading here is going to work though, unless I am missing something. So perhaps we can hash it out later today.
0585d8f
to
35f565d
Compare
2a4e165
to
614b611
Compare
614b611
to
dffc2e3
Compare
519055c
to
9273010
Compare
9273010
to
9f81bea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One little thing around responsibility. The crux being I think Store or storage layers should handle uniqueness constrains, like unique names. Services should just delegate to storage or talk to other services and so on.
Other than that I think this PR looks awesome. Happy to discuss that point and I will make sure to stick around and get it all cleared up so you can ship this today 👍
label/service.go
Outdated
} | ||
|
||
if upd.Name != "" { | ||
err := s.store.Update(ctx, func(tx kv.Tx) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this logic should live in Store and the *label.Store.UpdateLabel
should take the LabelUpdate.
Meaning all Service does is open a transaction and pass the right things down to storage.
My concern here is that after updating the name, we could fail validation of the label or fail to actually put the label, which is all store.UpdateLabel
is doing at the moment.
This would also reduce this function down to 1 transaction instead of opening 3.
So you could get a non-successfull call to this service to update the label, but the name did change successfully.
2507a09
to
b59e187
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff 🎉
b59e187
to
b8b20e7
Compare
Closes #18105
This PR adds the new label service to conform to the pattern established in the tenant and authorization services, where the http, middleware, service, and storage code is contained in the same package.
It also makes a couple of incidental changes, such as moving Metrics helper functions from the tenant and authorization packaged to their own file in kit (this could be moved somewhere else if there's a better place). Also changing
CreateUserResourceMappingForOrg
in kv from private to public (this can be changed back once URMs are removed from the Label Service).The KV Service passed to the new Label Service can also be removed once URMs are removed from the Label Service.
Next step will be to put this code behind a feature flag and incrementally test.