Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: parse JUJU_* env in one place #1313
feat: parse JUJU_* env in one place #1313
Changes from all commits
8b7eb0e
384fa9c
4f146be
2f4694a
a21a204
239a595
a296905
c7f7104
d9981e5
aad4788
982a2f7
f477b51
aced15d
b77f23e
e8b44d2
0d8fffc
fef2b9d
4020b6f
1d3d07d
a440143
b9cfbdc
922dffc
9f702d1
deef8c8
4733687
ccd3acb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Design question:
The half-baked idea I imagined was that
from_dict
returns aUnion
of types, and the caller always uses a match expression on the result. It would imply py3.10 (ubuntu 22.04) which isn't great, but I wonder (a) if it's a feasible design goal in the first place, and (b) if we should plan the design today withmatch
in mind.I.e. it would be great if this new class brought significant new added value over poking into
os.environ
directly.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.
Example: https://stackoverflow.com/a/71519690
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.
If I understand correctly, there is already some similar logic in
main.py
, specifically, in_get_event_args
, where it tries to match the type of the event then pull related environment variables.I think it would be great if we could somehow integrate it with JujuContext, maybe in the future we can build on top of this PR and implement that.
Regarding the scope of this PR, we had a short discussion today in the daily and we think this PR serves two purposes: 1, parse all ENV vars from a single place (previously it was scattered across
main.py
andframework.py
); and 2, provide some unified object so that others can build on top of it if they want to implement some experiments.So, for now, I'm sorry that there is no implementation of the event type matching thingy and we are going to merge this
_JujuContext
. Thanks!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.
If the intention is to find out which event Juju has emitted, then it's much simpler to just look at that directly rather than trying to match against which context variables have been set. If the intention is to get back a collection of types (e.g. which fields are set are the same with different events - start and stop would be setting the same) then I'm not sure I really see the use-case.
I don't think it's good to have a hierarchy of context objects when there's already a 1:1 mapping of what that would be in the hierarchy of event classes. A
SecretChangedJujuContext
would only be useful forSecretChangedEvent
, for example.As @IronCore864 mentioned,
_get_event_args
provides the "given a context, what are the arguments needed to create an event object". It would be simple enough for someone to subclass_JujuContext
to add ato_event
method (or whatever), if they wanted to build an "alt ops" package.What would be most useful in that sort of situation, I believe, is being able to create
SecretChangedEvent
(and on on) objects without opting in to the whole framework/handle system (and registering, in particular). My feeling is that's what we could provide (not this cycle) to continue making it easier for people to explore alternative ways to do event handling without re-implementing everything."significant" is subjective, of course, but I think there is value in not just collecting all the uses together as @IronCore864 mentioned, but also doing all the conversion so that you have a bunch of Python objects rather than a bunch of strings. With this, plus the similar hook tool wrapping in the model/model backend (the border of those could be a bit cleaner) that's the main interface with Juju (other than Pebble).