-
Notifications
You must be signed in to change notification settings - Fork 52
[WIP] Adding middleware support in factory #195
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?
Changes from 7 commits
1c10169
e827905
0ecadbe
614ca04
901b4b6
21f817a
25390d4
eea4356
aa50481
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,10 @@ | |
| import pkgutil | ||
| from collections import defaultdict | ||
|
|
||
| from pyeventsystem.interfaces import Middleware | ||
| from pyeventsystem.middleware import AutoDiscoveredMiddleware | ||
| from pyeventsystem.middleware import SimpleMiddlewareManager | ||
|
|
||
| from cloudbridge.cloud import providers | ||
| from cloudbridge.cloud.interfaces import CloudProvider | ||
| from cloudbridge.cloud.interfaces import TestMockHelperMixin | ||
|
|
@@ -12,6 +16,54 @@ | |
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| # Todo: Move to pyeventsystem if we're keeping this logic | ||
| class ParentMiddlewareManager(SimpleMiddlewareManager): | ||
|
|
||
| def __init__(self, event_manager=None): | ||
| super(ParentMiddlewareManager, self).__init__(event_manager) | ||
| self.middleware_constructors = [] | ||
|
|
||
| def add_constructor(self, middleware_class, *args): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could possibly be renamed to add_middleware_class, accepting the class in question, and both the constructor args and kwargs. |
||
| self.middleware_constructors.append((middleware_class, args)) | ||
|
|
||
| def remove_constructor(self, middleware_class, *args): | ||
| self.middleware_constructors.remove((middleware_class, args)) | ||
|
|
||
| def generate_simple_manager(self): | ||
| new_manager = SimpleMiddlewareManager() | ||
| for middleware in self.middleware_list: | ||
| new_manager.add(middleware) | ||
| for constructor, args in self.middleware_constructors: | ||
| m = constructor(*args) | ||
| new_manager.add(m) | ||
| for handler in self.get_subscribed_handlers(): | ||
| new_handler = handler.__class__(handler.event_pattern, | ||
| handler.priority, | ||
| handler.callback) | ||
| new_manager.events.subscribe(new_handler) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this means that events added to the factory later won't be subscribed to previously created providers? This is ok, but I think we need to clarify how exactly factory event subscriptions are meant to work and what the intended use cases are, so we know for sure that this is the correct behaviour.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was the original question that I had about this: whether we should make the provider object independent after it was created, or always treat it as a "child" of the factory. If the latter, it's definitely doable, but I would then suggest making the factory accessible through the provider as well. |
||
| return new_manager | ||
|
|
||
| # Removing install step from add. Since this manager is meant to create | ||
| # other managers rather than run. This will also simplify separating | ||
| # handlers added through middleware from those subscribed directly | ||
| def add(self, middleware): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should provide dual semantics here - one for shared middleware objects, and another for independent middleware, just to keep it simple. I'm currently leaning in favour of the factory namespacing child event dispatchers, and forwarding them as necessary, so that we still maintain event isolation within a given provider. I think we probably just need to avoid a scenario where providers are unwittingly subscribing to events outside of the provider. |
||
| if isinstance(middleware, Middleware): | ||
| m = middleware | ||
| else: | ||
| m = AutoDiscoveredMiddleware(middleware) | ||
| self.middleware_list.append(m) | ||
| return m | ||
|
|
||
| def get_subscribed_handlers(self): | ||
| handlers = [] | ||
| # Todo: Expose this better in pyeventsystem library | ||
| event_dict = self.events._SimpleEventDispatcher__events | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why we can't just access self.events?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| for key in event_dict.keys(): | ||
| for handler in event_dict[key]: | ||
| handlers.append(handler) | ||
| return handlers | ||
|
|
||
|
|
||
| class ProviderList(object): | ||
| AWS = 'aws' | ||
| AZURE = 'azure' | ||
|
|
@@ -27,9 +79,14 @@ class CloudProviderFactory(object): | |
| """ | ||
|
|
||
| def __init__(self): | ||
| self._middleware = ParentMiddlewareManager() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need a different name because ParentMiddlewareManager doesn't seem to explain what it's doing. Also, there is the issue of namespacing. And finally, I guess there's the issue of being able to intercept behaviour of a particular cloud (say openstack), and change behaviour for all classes of that cloud. One way would be to prefix the cloudname to the event. Another way would be is to check the instancetype of the sender: |
||
| self.provider_list = defaultdict(dict) | ||
| log.debug("Providers List: %s", self.provider_list) | ||
|
|
||
| @property | ||
| def middleware(self): | ||
| return self._middleware | ||
|
|
||
| def register_provider_class(self, cls): | ||
| """ | ||
| Registers a provider class with the factory. The class must | ||
|
|
@@ -136,7 +193,8 @@ def create_provider(self, name, config): | |
| 'A provider with name {0} could not be' | ||
| ' found'.format(name)) | ||
| log.debug("Created '%s' provider", name) | ||
| return provider_class(config) | ||
| return provider_class(config, | ||
| self.middleware.generate_simple_manager()) | ||
|
|
||
| def get_provider_class(self, name): | ||
| """ | ||
|
|
||
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 great, better than sending in middleware as a list.