-
Notifications
You must be signed in to change notification settings - Fork 9
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
Upgrade to Relation #19
Conversation
Up for discussion:
rather than:
This allows to:
On the other hand:
@simskij @benhoyt @Abuelodelanada @sed-i @rbarry82 @lucabello @dstathis thoughts? |
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.
Overall I like the approach, I think the UX is good.
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.
I left only a small nitpick, feel free to close it if you don't think it is doable/valuable.
I do indeed agree that your first suggestion makes the most sense. To me, a good rule of thumb usually is: If you can avoid args, do. |
This PR addresses several limitations of the existing Relation implementation:
1
we did not have enough data in the Event datastructure to mock the following relation-event-specific envvars
JUJU_UNIT_NAME
JUJU_APP_NAME
JUJU_DEPARTING_UNIT
-- only for *-relation-departedThis PR adds that functionality and corresponding tests.
2
We lacked facilities to correctly mock peer and subordinate relation types.
This PR adds two new types:
PeerRelation
andSubordinateRelation
to address that need.