Supporting app level relation data #232
Conversation
There was a problem hiding this comment.
Comments and suggestions inline. Mostly this is just missing a way to read the app level data from the remote side, but there is an issue with the way the request / response pattern is currently implemented that doesn't quite work with how Juju handles app level data (the permission denied issue).
Edit: The permission denied issue would be a problem for the general case, but the prometheus-manual interface is the only interface layer that I know of using that set of classes and for that case, non-leaders don't need to see the response so it's not a concern. Just ensure that the charm wraps any usage of this in an is-leader check and it should be fine.
There was a problem hiding this comment.
Apologies for the long lag time between reviews on this. I should note that this library is deprecated in favor of the Operator Framework, so doesn't really have a dedicated team or engineering time allocated for anything other than patches / bug fixes, so landing larger changes like this is likely to be slow. Also, I am still not fully convinced that my suggestion on the specific bug regarding forcing the request ID won't work. But either way I created a card for myself to track this and will try to stay on top of it better going forward.
|
I was thinking about this more, and since I'm 95% certain that the Request / Response pattern from the library is only used in the one interface implementation, maybe it would make sense to just move that part of the code out to live directly in the interface library and pare this PR down to just adding the app-level relation data support to the base |
Hello I think you meant I only need to submit for base Endpoint from previous commit and move request response changes to interface_prometheus_manual . so I removed code related to request_response. if there is anything I misunderstood, please correct me. Thanks a lot |
charms/reactive/relations.py
Outdated
| @@ -474,6 +474,7 @@ def set_state(self, state, scope=None): | |||
| See :meth:`conversation` and :meth:`Conversation.set_state`. | |||
| """ | |||
| self.conversation(scope).set_state(state) | |||
| set_flag = set_state | |||
There was a problem hiding this comment.
The formatting looks really weird on this file; I think there should be newlines and perhaps a comment to indicate that set_flag is just aliasing set_state
There was a problem hiding this comment.
Ah, I missed that these are still in here. They're actually method aliases, but they should have been cleaned up with the rest of the streamlining of the PR.
There was a problem hiding this comment.
Oops, these changes come from #233. I'm not sure why they're showing up in this PR.
Co-authored-by: Cory Johns <johnsca@gmail.com>
Co-authored-by: Cory Johns <johnsca@gmail.com>
Co-authored-by: Cory Johns <johnsca@gmail.com>
Co-authored-by: Cory Johns <johnsca@gmail.com>
remaining only endpoint
|
Released in 1.5.0 |
Squashed commit notes: Fixing a bug from [2]. * Fixing wrong reference * remove 3.5 test Driven by: * relation_id is in Endpoint class, but self.relation. returns relation.so it should be self.relation_id instead of self.relation.relation_id * LP[1] is the cause of this implementation. ( prior patch was : [2]) [1]: LP : https://bugs.launchpad.net/charm-kubernetes-master/+bug/1899706 [2]: #232
I need some advice to implement this.
Please review my code if it is proper or there is good way