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(testing): set JUJU_REMOTE_APP and allow fetching relation data in relation-broken #1130

Merged

Conversation

tonyandrewmeyer
Copy link
Contributor

As far as the Juju team and I have been able to tell (see comments in the Juju issue) JUJU_REMOTE_APP is always set in relation-broken events.

This PR removes the Harness behaviour that simulates JUJU_REMOTE_APP not being available (which was never consistently the case anyway) and the model code that blocks getting remote data during relation-broken (which is discouraged in the case where the entire integration is going away, but is permitted).

Fixes #1128.

@tonyandrewmeyer
Copy link
Contributor Author

Note that the canonical/nginx-ingress-integrator-operator tests pass under this branch.

@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review February 25, 2024 23:17
@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Feb 25, 2024

The spacing changes in pebble.py and docs/conf.py are ones that Ruff wants. This seems to be a change in 0.2.2 (#9266), but I'm not sure why it didn't show up in the PR that bumped ruff to 0.2.2. I don't love the Pebble change, but I think I prefer it to a bunch of noqas in the same place.

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks for this fix.

@benhoyt
Copy link
Collaborator

benhoyt commented Feb 26, 2024

@jameinel Would be good to have your eyes on this too, seeing we've discussed this before and you commented on the issue.

@tonyandrewmeyer
Copy link
Contributor Author

The spacing changes in pebble.py and docs/conf.py are ones that Ruff wants. This seems to be a change in 0.2.2 (#9266), but I'm not sure why it didn't show up in the PR that bumped ruff to 0.2.2. I don't love the Pebble change, but I think I prefer it to a bunch of noqas in the same place.

This was figured out in #1134 - the version was ~0.2.1 previously, and new rules were introduced in 0.2.2. That PR takes care of this, but will leave the adjustments here as well.

Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the changes.
I didn't do the Juju side testing to confirm that for all supported Juju versions this is the expected behavior. But I think it is the behavior that we want to have.

raise KeyError(
'Cannot index relation data with "None".'
' Are you trying to access remote app data during a relation-broken event?'
' This is not allowed.')
return self._data[key]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would we want to have a different check in case key is None? Just to avoid weird KeyError failures.
I don't know that it is necessary, but I like giving better errors when we can.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tony and I discussed, and we'd like our principle to be "we've invested in type annotations, let's go all in on static type checking and let that catch this." Most charming teams are using MyPy and PyRight already, and those that aren't, I think we should nudge them in that direction.

@@ -2167,21 +2167,9 @@ def relation_remote_app_name(self, relation_id: int) -> Optional[str]:
if relation_id not in self._relation_app_and_units:
# Non-existent or dead relation
return None
if 'relation_broken' in self._hook_is_running:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is _hook_is_running used for anything else? or was this the use case and we can pull it out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used for bypassing access control on relation data as well.

@tonyandrewmeyer
Copy link
Contributor Author

I should do a CHANGES entry for this I think.

@tonyandrewmeyer
Copy link
Contributor Author

I've run this branch against the unit tests of 132 charms (all passing with current main of ops) and they all continue to pass.

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do this!

ops/model.py Outdated Show resolved Hide resolved
@benhoyt benhoyt changed the title JUJU_REMOTE_APP seems to always be provided in relation-broken (in Ju… fix(testing): set JUJU_REMOTE_APP and allow fetching relation data in relation-broken Feb 27, 2024
@benhoyt benhoyt merged commit c623461 into canonical:main Feb 27, 2024
28 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the remove-relation-broken-special-case-1128 branch February 27, 2024 23:40
@carlcsaposs-canonical
Copy link
Contributor

JUJU_REMOTE_APP is always set in relation-broken events

@tonyandrewmeyer is this a recent change? I remember encountering an occasional issue on juju 2.9 where JUJU_REMOTE_APP was not set (depends on race with relation-broken event on other charm)

@tonyandrewmeyer
Copy link
Contributor Author

JUJU_REMOTE_APP is always set in relation-broken events

@tonyandrewmeyer is this a recent change? I remember encountering an occasional issue on juju 2.9 where JUJU_REMOTE_APP was not set (depends on race with relation-broken event on other charm)

The Juju team couldn't find a way to reproduce it (they tried recently). Most significantly, there was consensus that it should be always set, so having ops try to be consistent or work around a bug wasn't a good move.

If you know/find a way to reproduce, please open a Juju bug (or re-open the old one) with steps and we can make sure it gets fixed there.

@carlcsaposs-canonical
Copy link
Contributor

If you know/find a way to reproduce, please open a Juju bug (or re-open the old one) with steps and we can make sure it gets fixed there.

sorry for the delay, didn't have a chance to try to reproduce this until now

was able to reproduce, steps in Juju bug: https://bugs.launchpad.net/juju/+bug/1960934/comments/15

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.

Harness should always provide the remote app name in relation-broken
4 participants