Skip to content
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

fix: remote unit data is available in relation-departed #1364

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tonyandrewmeyer
Copy link
Contributor

During relation-departed and relation-broken, the remote unit data should still be available to the charm, but the relation is no longer included in relation-list, so it's not available by the usual means (but is available when using the hook tool directly).

When a remote unit name is provided in the Juju context, ensure that unit is included in the relation units.

Fixes #1109

@tonyandrewmeyer
Copy link
Contributor Author

This can be done much more simply by adding this at line 208 of _main.py:

        if relation:
            relation.data._data[kwargs['unit']] = ops.model.RelationDataContent(
                relation, kwargs['unit'], model._backend
            )

Pros/cons compared to the current state of the branch (I've manually tested that both work, but unit tests are non-trivial so want to finalise on an approach before committing to those):

  • This is far less invasive, and it feels wrong that Model has the remote unit name (although it's not particularly different from knowing the broken relation ID, which we already did).
  • This changes the event.relation object but if other code does model.get_relation during the relation-departed event, they don't get the change. It feels like get_relation shouldn't need to know anything about the current event but it also feels wrong for the two objects to be different.

The more I work on this, the more it feels like we're working around Juju behaviour and perhaps we should be advocating for a change in Juju instead (to only change relation-list at the conclusion of relation-broken).

@dimaqq
Copy link
Contributor

dimaqq commented Sep 25, 2024

@tonyandrewmeyer still draft or ready for review and possibly merge?

@tonyandrewmeyer
Copy link
Contributor Author

@tonyandrewmeyer still draft or ready for review and possibly merge?

Still draft, expecting to get back to this in the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remote unit data is inaccessible in relation-departed
2 participants