-
Notifications
You must be signed in to change notification settings - Fork 121
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
docs: use 'integrate with' rather than 'relate to' #1145
docs: use 'integrate with' rather than 'relate to' #1145
Conversation
relations will trigger `RelationCreatedEvent` before :class:`StartEvent` is | ||
emitted. |
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.
This is a drive-by: the existing wording is quite unclear (to me, at least). I think the revised wording is what is meant?
@@ -521,7 +522,7 @@ class RelationJoinedEvent(RelationEvent): | |||
This event is triggered whenever a new unit of a related | |||
application joins the relation. The event fires only when that | |||
remote unit is first observed by the unit. Callback methods bound | |||
to this event may set any local unit settings that can be | |||
to this event may set any local unit data that can be |
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.
This (and the below) are another drive-by: I only recall seeing "settings" meaning the content of the relation databag in the oldest documentation, and everywhere else in the API docs we refer to "data", so I believe this is much clearer.
@@ -1339,7 +1339,7 @@ def set_leader(self, is_leader: bool = True) -> None: | |||
|
|||
If this charm becomes a leader then `leader_elected` will be triggered. If :meth:`begin` | |||
has already been called, then the charm's peer relation should usually be added *prior* to | |||
calling this method (with :meth:`add_relation`) to properly initialize and make | |||
calling this method (with :meth:`add_relation`) to properly initialise and make |
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.
One last wee drive-by.
In #1133:
I think that this has too little value to outweigh the overhead of having a new name and keeping an alias for backwards compatibility. Likewise, I think |
The unit is unable to progress to an active state because an application with which | ||
it is integrated is not running. |
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.
@benhoyt this seems very specific about the use-case for WaitingStatus
. Isn't it ok to use it if you're waiting on anything, rather than specifically for an integration/relation?
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.
That's taken straight from the comment in Juju core/status/status.go, so I presume it's intentional:
// The unit is unable to progress to an active state because an application to
// which it is related is not running.
I think you'd use MaintenanceStatus otherwise.
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.
That's taken straight from the comment in Juju core/status/status.go, so I presume it's intentional:
Ah, ok. It should presumably stay in that case.
I think you'd use MaintenanceStatus otherwise.
Hmm, if I'm waiting for the container to be ready or waiting for an API to come up, WaitingStatus
seems the natural one to use. MaintenanceStatus
seems like it's more when the charm is actively doing something.
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.
Thanks. I think this is fine. I'm sure there are further improvements we could make, but this is a good start.
The unit is unable to progress to an active state because an application with which | ||
it is integrated is not running. |
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.
That's taken straight from the comment in Juju core/status/status.go, so I presume it's intentional:
// The unit is unable to progress to an active state because an application to
// which it is related is not running.
I think you'd use MaintenanceStatus otherwise.
The current Juju terminology is that apps "integrate" to form a "relation". This PR tweaks the API documentation to align with that wording.
Fixes: #1133