-
-
Notifications
You must be signed in to change notification settings - Fork 191
Simplify the MDS Builder API #620
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
base: main
Are you sure you want to change the base?
Conversation
eea904d
to
26811a4
Compare
0803765
to
9aeaa4b
Compare
Please note, this PR does not actually add any new feature to the project. It does however help clarify how existing features can solve a users problem. Does this suit your need @Simonl9l, @joegoldman2. Also curious if you have any thoughts @iamcarbon and @aseigler. I've left it out of this PR, but I am interested in writing a |
9aeaa4b
to
63d58c8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #620 +/- ##
==========================================
+ Coverage 77.55% 78.34% +0.79%
==========================================
Files 98 98
Lines 2539 2540 +1
Branches 422 422
==========================================
+ Hits 1969 1990 +21
+ Misses 460 440 -20
Partials 110 110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Checking in again if this solves your previous issues @Simonl9l, @joegoldman2? |
@abergs we’re currently on a different path in this regard, and are no longer actively/currently using you package so defer to others. thanks though for all you work on this package. |
@abergs this new API is more user-friendly, thank you for that. However, there is still something I don't really like and understand. The service is still strongly coupled to the cache, so it's mandatory to register |
This PR aims to simplify the MDS builder API and adress some current problems we're seeing.
Developers today seems to believe it's hard / unfeasible to implement their own "caching" layer for MDS data.
This is because we have not documented the current architecture properly, and the current builder pattern does indicate that there is not a lot of flexibility in the design, although the opposite is true.
This PR adds documentation, as well as reshapes the API to better surface the flexibility.
Example in the API change
Before
After:
We're also adding a two new builder methods, that makes it clear that you can register your own MDS Service to wrap the repositories, as well as custom repositories
Using FIDO mds repository + your own custom service to do caching, logging, storage, whatever
Using a completely bespoke solution to fetch mds data and access it
Breaking changes
This PR introduces some breaking changes in the API builder (compile time) and some run time breaking changes, namely: Removes the default registration of the NullMetadataService
IFido2MetadataServiceBuilder
NullMetadataService
We are also adding plenty of comments as well as hiding the ConformanceMDS from Intellisense. I wonder if we should really remove it instead?