-
Notifications
You must be signed in to change notification settings - Fork 10
feat: Data Interfaces V1 #241
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
feat: Data Interfaces V1 #241
Conversation
tests/v1/integration/backward-compatibility-charm/src/charm.py
Dismissed
Show dismissed
Hide dismissed
tests/v1/integration/database-charm/lib/charms/data_platform_libs/v1/data_interfaces.py
Fixed
Show fixed
Hide fixed
tests/v1/integration/database-charm/lib/charms/data_platform_libs/v1/data_interfaces.py
Fixed
Show fixed
Hide fixed
tests/v1/integration/database-charm/lib/charms/data_platform_libs/v1/data_interfaces.py
Fixed
Show fixed
Hide fixed
tests/v1/integration/database-charm/lib/charms/data_platform_libs/v1/data_interfaces.py
Fixed
Show fixed
Hide fixed
tests/v1/integration/database-charm/lib/charms/data_platform_libs/v1/data_interfaces.py
Fixed
Show fixed
Hide fixed
tests/v1/integration/database-charm/lib/charms/data_platform_libs/v1/data_interfaces.py
Fixed
Show fixed
Hide fixed
tests/v1/integration/database-charm/lib/charms/data_platform_libs/v1/data_interfaces.py
Fixed
Show fixed
Hide fixed
tests/v1/integration/database-charm/lib/charms/data_platform_libs/v1/data_interfaces.py
Fixed
Show fixed
Hide fixed
tests/v1/integration/database-charm/lib/charms/data_platform_libs/v1/data_interfaces.py
Fixed
Show fixed
Hide fixed
# This is the 1st commit message: feat: DPV1 # The commit message #2 will be skipped: # feat: unit tests + linting # The commit message #3 will be skipped: # fix: linting # The commit message #4 will be skipped: # fix: enable tests # The commit message #5 will be skipped: # fix: only correct tests # The commit message #6 will be skipped: # fix: application deployed # The commit message #7 will be skipped: # fix: build charms
ec4f57a to
ef78f52
Compare
This is only for Juju 3 charms
There's is no backward compatibility with pre-secret charms, so no compatibility with the older versions of the library, this was explicitely requested by Mykola. |
Thanks for the clarification. |
|
+1 here from Cassandra folks! 🥳 We have integrated the library successfully on cassandra. Is there any timeline when this PR is going to be merged or should we just go ahead and merge the Cassandra PR? I suppose we can always update the lib once it is consolidated |
@deusebio any reason why you did not migrate the peer relation data handling to v1? |
| repository_type: type[TRepository], | ||
| model: type[TCommon] | TypeAdapter | None, | ||
| ): | ||
| self.charm = charm |
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.
As we discussed, charm can be removed from this class, as it is not used. This avoids having to send the charm object to data model focused classes.
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.
Updated, hopefully I forgot no use case.
I'd say to keep the PR confined and capacity. My assumption (also looking at the spec DA175) is that data-interfaces were mostly concerned (and critical) for client<>server relations. Of course we could also apply them to peer, but that's nice to have for us right now. We have a few peer relations, so this may not be a trivial change. The current task we need to deliver before end of cycle are implementing client relations (the PR attached) and implement HA tests. There is not much time left before the end of the cycle and we would like to wrap those up. TLDR I believe using a more structured approach for peer can also be useful, but probably to be addressed in the future |
|
@deusebio @zmraul @skourta: I've updated the interfaces following the comments from @zmraul : @zmraul Looks better ? |
* SecretStr and SecretBool where nice to prevent errors but removed the abstraction of secrets in the public interface. * Refacto the models to extract the serialization of the secrets in a specific class and allow to have custom classes depending on this base common model
…m-libs-data-interfaces-v-1-implementation
Co-authored-by: Smail KOURTA <[email protected]>
reneradoi
left a comment
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.
Thank you @Gu1nness for all your effort and your work on this PR! I approve, no blockers from my side.
Yes, thank you! This is a nice improvement :) |
imanenami
left a comment
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 already fabulous for the first release of V1, and we have integrated it successfully with the main Kafka charm, so I'm happy with it. Great effort @Gu1nness!
skourta
left a comment
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.
Thank you, @Gu1nness. Great work! All needed features and helpers to have full migration to v1 are now included.
Mehdi-Bendriss
left a comment
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.
Outstanding work Neha!
Implementation of Data Interfaces V1:
This V1 supports multiple requests from the requirer, and supports both bulk events (1 event for all the requests at the same time) and single events (one event per request), according to your usecase.
It supports credentials updates from the provider (used by some charms), it supports MTLS from the requirer, and heavily relies on type systems and pydantic!
Example of use