fix: Improved handling within ProviderRegistry #560
Conversation
It is incorrect to call `ProviderRegister.set_provider` without a provider AND a domain. A validation check exists for the provider, but none for the domain. In this commit, we introduce that domain validation and introduce tests to capture this expected behavior. Signed-off-by: Matthew Keeler <[email protected]>
If a provider is set for two different domains, it will only be initialized once. However, if a customer were to use a provider as both default and domain specific, it would incorrectly be initialized twice. Signed-off-by: Matthew Keeler <[email protected]>
If a provider is set for two different domains, and then one of them is replaced, it will only be shutdown once. However, if a customer were to use a provider as both default and domain specific, replacing one or the other would incorrectly shutdown both usages. Signed-off-by: Matthew Keeler <[email protected]>
Once a provider has been removed, there is no value in retaining a reference to it in the `_provider_status` dictionary. Instead of explicitly setting the status as `NOT_READY`, we can rely on this being the default status served from the `get_provider_status` method, allowing us to remove the dictionary entry entirely. Signed-off-by: Matthew Keeler <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #560 +/- ##
==========================================
+ Coverage 98.01% 98.18% +0.17%
==========================================
Files 41 41
Lines 1913 1982 +69
==========================================
+ Hits 1875 1946 +71
+ Misses 38 36 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gruebel
left a comment
gruebel
left a comment
There was a problem hiding this comment.
overall looks good, but as stated I'm not sure about the shutdown change
Signed-off-by: Matthew Keeler <[email protected]>
keelerm84
force-pushed
the
feat/registry-improvements
branch
from
23c6877 to
69076e2
Compare
gruebel
left a comment
gruebel
left a comment
There was a problem hiding this comment.
Nice work, thanks 🍻 only thing missing is fixing DCO issue? Did you forget to sign-off a commit?
|
Thank you @keelerm84, overall looks good! Can you open a separate PR for the changes in the first commit though? Since we are squash merging by default I prefer to not batch merge several changes together with different sets of risks. The first set of changes we can merge pretty quickly while we focus our energy on the other ones. |
|
@federicobond first commit as a PR is now open at #561 |
keelerm84
force-pushed
the
feat/registry-improvements
branch
from
095fe12 to
4550e53
Compare
Signed-off-by: Matthew Keeler <[email protected]>
keelerm84
force-pushed
the
feat/registry-improvements
branch
from
4550e53 to
d336554
Compare
Signed-off-by: Matthew Keeler <[email protected]>
|
@federicobond that first commit has been merged separately. I have updated the PR description and RP override block to remove mention of it. Let me know if you want me to separate out any other bits of functionality. |
3 participants
This PR
Introduces the following changes to the
ProviderRegistry.All of this functionality is in this single PR, but this can be split into individual PRs if desired. Each commit contains a singular bit of functionality or change, so reviewing individually might be easier.
Assuming we keep the PR as one, I have provided the changelog override necessary for release please.
BEGIN_COMMIT_OVERRIDE
refactor: Delete provider status instead of marking as NOT_READY
fix: Prevent providers from being shutdown multiple times
fix: Prevent providers from being initialized multiple times
END_COMMIT_OVERRIDE