-
Notifications
You must be signed in to change notification settings - Fork 14
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
DM-41235: Make SqlRegistry a concrete class #898
Conversation
Updated test_server.py to skip all tests as it depends on RemoteRegistry. We will fix the tests later using new shiny client/server Butler.
SqlRegistry un-inherits Registry and drops an intermediate _ButlerRegistry interface. All clients of _ButlerRegistry now use SqlRegistry. Registry is still an interface for client-facing `Butler.registry`, which is a shim for SqlRegistry or other butler methods.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #898 +/- ##
==========================================
- Coverage 87.59% 87.23% -0.37%
==========================================
Files 272 270 -2
Lines 36584 36345 -239
Branches 7637 7598 -39
==========================================
- Hits 32046 31704 -342
- Misses 3351 3484 +133
+ Partials 1187 1157 -30
☔ View full report in Codecov by Sentry. |
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.
Looks okay although I am now wondering what the purpose of Registry
ABC is.
for filename in filenames: | ||
with open(os.path.join(TESTDIR, "data", "registry", filename)) as stream: | ||
# Go behind the back of the import code a bit to deal with | ||
# the fact that this is just registry content with no actual | ||
# files for the datastore. | ||
backend = YamlRepoImportBackend(stream, butler.registry) | ||
backend = YamlRepoImportBackend(stream, butler._registry) |
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 is because butler.registry
is no longer a registry but butler._registry
is?
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.
Mostly because I changed YamlRepoImportBackend
to accept SqlRegistry
instead of Registry
. I think that Registry
at this point exists only because we need backward-compatible interface for Butler.registry
. When we move all functionality to Butler
we will eventually remove Registry
completely.
This is for clients that still use |
OTOH if we need registry interface for remote butler then its implementation will likely be different from RegistryShim. We probably should keep Registry ABC for now just in case. |
SqlRegistry un-inherits Registry and drops an intermediate _ButlerRegistry
interface. All clients of _ButlerRegistry now use SqlRegistry. Registry is
still an interface for client-facing
Butler.registry
, which is a shim forSqlRegistry or other butler methods.
Checklist
doc/changes