-
Notifications
You must be signed in to change notification settings - Fork 6
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
[ENG-5196] addon capabilities (in api and implementers' toolkit) #12
Conversation
ea018db
to
f21d344
Compare
4acb163
to
b442bce
Compare
d76f372
to
92f07f6
Compare
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.
Still working to groc the toolkit, but here's some quick feedback on everything before that
from addon_service.common.base_model import AddonsServiceBaseModel | ||
|
||
|
||
class AuthorizedStorageAccount(AddonsServiceBaseModel): | ||
# TODO: authorized_capabilities = ArrayField(...) | ||
authorized_capabilities = ArrayField( | ||
models.IntegerField(choices=IntStorageCapability.as_django_choices()), |
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.
Nit: would rather enforce this differently, since setting it in choices
requires migrations when options change.
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.
fair enough; how about a validator?
@@ -74,6 +76,7 @@ def __init__(self, *args, **kwargs): | |||
|
|||
def create(self, validate_data): | |||
account_owner = validate_data["account_owner"] | |||
authorized_capabilities = validate_data["authorized_capabilities"] |
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.
nit: can we change validate_data
-> validated_data
here? feels weird to have a variable with a verb clause as a name (I think my brain just autocorrected this when it first made it in)
__all__ = ("IntStorageCapability",) | ||
|
||
|
||
class IntStorageCapability(IntEnumForEnum, base_enum=StorageCapability): |
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.
What's the purpose between the dual enums? Feels very complicated. On OSF, I was able to handle the translation from int to string for the API with a single enum here
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.
hmm maybe it is too complicated, it sure did get finicky... bloomed from a conviction that addon_toolkit
should be completely unaware of addon_service
's database details like storing enum as int, but also that addon_toolkit.storage
should define the source-of-truth StorageCapability
enum, and then maybe got overexcited about having a use for __init_subclass__
kwargs
also seemed nice code-aroma to use enums as explicit source-controlled maps between a python-name and both api and database representations -- the single-enum requires the api representation to be the python name... which may be fine, really
for the operations api i did further complicate the idea, but may have better expressed intent?
included in #16 |
add the concept of "capabilities" that represent statically declared groups of "operations" for a category of addons.
code organization
addon_toolkit
: (new) holds declared addon category interfaces (for addon implementers to subclass) and helpful tools for interacting with themaddon_service
: the existing django app that usesaddon_toolkit
and available addon implementationsaddon implementers should import from
addon_toolkit
, but notaddon_service
(to avoid cyclical confusion)api changes
two new attrs in api (both lists of strings)
authorized-storage-accounts
:authorized_capabilities
configured-storage-addons
:connected_capabilities
"access"
and"update"
database changes
two new model fields in api (both
ArrayField(IntField)
with anIntEnum
for choices (guaranteed to map to/from the string-based enum used for api values))AuthorizedStorageAccount
:authorized_capabilities
ConfiguredStorageAccount
:connected_capabilities
how to...
...add a category of addons (see
addon_toolkit.storage
for an incomplete example):enum.Enum
with string values@addon_interface(capabilities=MyCapabiltiesEnum)
)@proxy_operation(capability=...)
or@redirect_operation(capability=...)
)...implement an addon of a given category:
addon_toolkit.storage.StorageAddon
)future work
Configured*Addon