feat: poc parsing juju env in one place - alternative draft #1314
Conversation
chore: poc parsing juju env in one place chore: refactor _JuJuContext to a dataclass
There was a problem hiding this comment.
While in general, "flat is better than nested", I think that 1 level of nesting is just fine, assuming that the net difference is, roughly, writing something like
assert ctx.notice
if ctx.notice.type == "...": ...
instead of
assert ctx.notice_id
if ctx.notice_type == "...": ...
On the other hand, if nesting grew deeper, I wouldn't be so sure.
| juju_action: '_JujuAction' | ||
| juju_charm_dir: Optional[str] | ||
| juju_debug: Optional[str] | ||
| juju_departing_unit: str |
There was a problem hiding this comment.
I think that this field should be scoped under relation
| juju_dispatch_path: Optional[str] | ||
| juju_notice: '_JujuNotice' | ||
| juju_pebble_check_name: str | ||
| juju_relation: '_JujuRelation' |
There was a problem hiding this comment.
I would prefer that the top-level field is optional, rather than its components
| juju_remote_app: str | ||
| juju_remote_unit: str | ||
| juju_secret: '_JujuSecret' | ||
| juju_storage_id: str |
There was a problem hiding this comment.
🤔 this field is called "storage tag" in juju codebase. The change is from 3 years ago, maybe we can follow that lead? wdyt?
| juju_remote_unit: str | ||
| juju_secret: '_JujuSecret' | ||
| juju_storage_id: str | ||
| juju_workload_name: str |
There was a problem hiding this comment.
I wonder if this should be scoped under "container"
| juju_departing_unit: str | ||
| juju_dispatch_path: Optional[str] | ||
| juju_notice: '_JujuNotice' | ||
| juju_pebble_check_name: str |
There was a problem hiding this comment.
I imagine this too should be scoped under... a check? a container? a notice? Ugh this is getting difficult...
There was a problem hiding this comment.
It indeed gets messy. It shouldn't really be under notice because notices are meant to be an implementation detail of check events. It's really check and notice both under container, but I don't think we want to be three levels deep (I'm not even convinced of two levels).
There was a problem hiding this comment.
Yeah, I'm with Tony on this one, and this is part of the reason that hierarchies -- while seeming enticing at first -- are often not ideal. Sometimes it's unclear which hierarchy something belongs in, and sometimes a thing belongs in multiple hierarchies.
In addition, if we're going for nested, I don't like how we have to have all these different classes, like _JujuRelation (and it's actually not a relation, it's an "extended relation identifier" or something).
So I'm much more in favour of flat, 1-level for this, as #1313 does.
There was a problem hiding this comment.
Additionally, we convert to "proper" types further down, like converting notice id/type/key to LazyNotice in the notice events. I think that's another reason to keep the types here flat and simple.
|
After discussion, we will use a flat structure without nested objects, and we will put all fields as |
Parse all
JUJU_*env vars in one place.Closes #1075.
Alternative version where variables are grouped with explicit types.
See the first version here: #1313.
Typing is explicitly not
Noneexceptjuju_charm_dir,juju_debug, andjuju_dispatch_pathbecause the original code tests whether they are inos.environor not.