Skip to content
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

Merged
merged 1 commit into from
May 21, 2020
Merged

refactor: add new label package #18078

merged 1 commit into from
May 21, 2020

Conversation

AlirieGray
Copy link
Contributor

@AlirieGray AlirieGray commented May 13, 2020

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.

  • Rebased/mergeable
  • Tests pass

@AlirieGray AlirieGray force-pushed the refactor/label-service branch from 8d9f68a to 9eafd46 Compare May 13, 2020 08:10
@AlirieGray AlirieGray changed the title Refactor/label service Refactor: add new label service May 13, 2020
@AlirieGray AlirieGray changed the title Refactor: add new label service Refactor: add new label package May 13, 2020
@AlirieGray AlirieGray changed the title Refactor: add new label package refactor: add new label package May 13, 2020
@AlirieGray AlirieGray force-pushed the refactor/label-service branch 12 times, most recently from 96b9fa7 to f0e9840 Compare May 18, 2020 20:09
@AlirieGray AlirieGray requested review from lyondhill and GeorgeMac May 18, 2020 20:11
@AlirieGray AlirieGray force-pushed the refactor/label-service branch from f0e9840 to 4778612 Compare May 18, 2020 20:15
Copy link
Contributor

@GeorgeMac GeorgeMac left a 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.

@AlirieGray AlirieGray force-pushed the refactor/label-service branch 8 times, most recently from 0585d8f to 35f565d Compare May 19, 2020 20:18
@AlirieGray AlirieGray force-pushed the refactor/label-service branch 2 times, most recently from 2a4e165 to 614b611 Compare May 19, 2020 20:25
@AlirieGray AlirieGray force-pushed the refactor/label-service branch from 614b611 to dffc2e3 Compare May 19, 2020 21:01
@AlirieGray AlirieGray force-pushed the refactor/label-service branch 2 times, most recently from 519055c to 9273010 Compare May 20, 2020 19:20
@AlirieGray AlirieGray requested a review from GeorgeMac May 20, 2020 20:01
@AlirieGray AlirieGray force-pushed the refactor/label-service branch from 9273010 to 9f81bea Compare May 20, 2020 20:22
Copy link
Contributor

@GeorgeMac GeorgeMac left a 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 {
Copy link
Contributor

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.

@AlirieGray AlirieGray force-pushed the refactor/label-service branch 4 times, most recently from 2507a09 to b59e187 Compare May 21, 2020 16:43
Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff 🎉

@AlirieGray AlirieGray force-pushed the refactor/label-service branch from b59e187 to b8b20e7 Compare May 21, 2020 16:48
@AlirieGray AlirieGray merged commit 7f4ddab into master May 21, 2020
@AlirieGray AlirieGray deleted the refactor/label-service branch May 21, 2020 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Label Service
3 participants