Remove SDI dependency from ingress_per_unit #35
Conversation
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
1 similar comment
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
| elif self.is_failed(event.relation): | ||
| self.on.failed.emit(event.relation) | ||
|
|
||
| @cache |
There was a problem hiding this comment.
Seems likely that a charm's status may change within a single charm code invocation. If not now then perhaps in the future. Using cache here means having an implicit assumption of a particular status handling approach: it must be updated before it is ever checked.
There was a problem hiding this comment.
It makes it also hard(er) to test these methods, so I'm all +1 for removing it. I kept it because it was in SDI, and within that context it made sense because SDI managed its own status and could guarantee that it would never change within a charm's execution lifetime. But now that we own it, and if we think it might change in the future... Fair enough. Also it's not that big an optimization to warrant all the extra potential bugginess and mental overhead
There was a problem hiding this comment.
@mmanciop according to the same spirit, what'd you think about removing all caching in IPU and be done with it?
| # validation step 2: only leaders can write app data | ||
| if entity is this_app and not this_unit.is_leader(): | ||
| raise RelationPermissionError(relation, this_unit) |
There was a problem hiding this comment.
Can this case be covered by a regular leader guard at the beginning of the function?
And why raise in that case?
|
|
||
| def is_unit_ready(self, unit: Unit) -> bool: | ||
| """Returns whether the given unit has provided its name, model, host and port data.""" | ||
| self._check_unit_belongs_to_relation(unit) |
There was a problem hiding this comment.
Would it make more sense to have this return a bool instead of raising?
If not, then should probably add a Raises: statement.
But shouldn't is_ready return a bool without raising regardless?
There was a problem hiding this comment.
@mmanciop what was the idea of making it raise?
I see most of the methods which use _check_unit_belongs_to_relation return Optional[Trype], so wouldn't it make more sense to do:
if not self._unit_belongs_to_relation(unit):
return None
or do we want the user to know that there's something deeply wrong with what they're trying to do?
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
6 similar comments
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
| @@ -14,9 +14,7 @@ | |||
| charm's `requirements.txt`.** | |||
|
|
|||
| ```shell | |||
| cd some-charm | |||
| charmcraft fetch-lib charms.traefik_k8s.v0.ingress_per_unit | |||
There was a problem hiding this comment.
Unless we break the library into two (provides/requires) will charmers not need to add jsonschema to their requirements.txt?
There was a problem hiding this comment.
Good point. How does that work for libraries? Do we have a way for libs to declare their dependencies?
There was a problem hiding this comment.
Now that's interesting. I would probably use log.warning, but I quite like the idea that it still has a chance of working...
There was a problem hiding this comment.
either way, you still need to include in the docs to echo jsonschema into requirements.txt or similar :)
Other option is just to validate on the Traefik side, but that will be less reliable.
There was a problem hiding this comment.
yeah the catch here is that both the provider and the requirer have to install that dependency. Or indeed we could say that Traefik owns the correctness of the data, so the requirer is responsible for doing its own validation if it so desires; and we can provide a utility method or just the public schema for "this is what you should be expecting". Then the charm using the requirer side would have to also install that dependency.
| elif self.is_failed(event.relation): | ||
| self.on.failed.emit(event.relation) | ||
|
|
||
| @cache |
| """Check whether the given relation is available. | ||
|
|
||
| Or any relation if not specified. | ||
| """ |
There was a problem hiding this comment.
I would be content to drop the returns piece in the docstring and rely upon type annotations unless the return type needs further explanation
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
6 similar comments
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
Remove SDI dependency from ingress_per_unit, keeping backwards compatibility between the new version of the IngressPerUnitProvider and SDI-based IngressPerUnitRequirer implementations. Also: solve a bug that prevented follower units of traefik-k8s from correctly generating route configurations.
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
Juju does not expose the RELATION_NAME over the relation_broken events (see $1) and that causes bad behaviors with OF (see $2), that we need to guard against. Closes #34 $1 https://bugs.launchpad.net/juju/+bug/1960934 $2 canonical/operator#693
|
Libraries are not up to date with their remote counterparts. If this was stdoutstderr |
Removed SDI dependency from v0/ingress_per_unit Provider/Requirer
Fixes #24