-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add actions to control the new workload #54
Conversation
Nice PR! Some points:
|
There's not a a lot of added logic in the charm for now, everything is mostly handled by the snap who has its own tests.
This will be done in the next cycle for sure |
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.
LGTM
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.
A few comments
@@ -31,7 +33,7 @@ class PeerRelationNetworkUnavailableError(exceptions.BindCharmError): | |||
"""Exception raised when the peer relation network is unavailable.""" | |||
|
|||
|
|||
class BindCharm(ops.CharmBase): | |||
class BindCharm(actions_mixin.ActionsMixin, ops.CharmBase): |
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.
I am not sure about the use of multiple inheritance here using a mixin, as it is not going to be reused in other places.
As ActionsMixin
only uses three variables (charm.on
, charm.bind
and charm.framework
) I think an instance of tha class could be created with those variables explicitly passed, and so BindCharm and ActionsMixin are a bit less coupled.
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.
IMO, if we pass and instance of the class, they are tightly coupled. I'm doing the mixin dance to acknowledge it but still split the code in two.
This is in contrast to the bind service who can be tested outside of the charm instance.
self.snap_path: str = "" | ||
try: | ||
self.snap_path = str(self.model.resources.fetch("charmed-bind-snap")) | ||
except ops.ModelError as e: |
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.
Will this work without charmed-bind-snap
?
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.
Yes, the CI is working without it. I'm just using it locally
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.
Okay, my bad. Doesn't work in the last CI anymore. Fixing...
Test coverage for 75c5864
Static code analysis report
|
Closing this as ACLs will be part of a DNS policy charm |
Overview
Checklist
src-docs
urgent
,trivial
,complex
)