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

feat!: add Scenario classes that match the ops status classes #142

Merged
merged 29 commits into from
Jul 17, 2024

Conversation

tonyandrewmeyer
Copy link
Collaborator

@tonyandrewmeyer tonyandrewmeyer commented Jun 7, 2024

Adds classes that match the ops status classes:

  • UnknownStatus
  • ActiveStatus
  • WaitingStatus
  • MaintenanceStatus
  • BlockedStatus
  • ErrorStatus

These are comparable with (both == and isinstance) the ops classes, so this is a backwards compatible change (except for static checking), but the expectation is that instead of this code:

import ops, scenario

state_in = scenario.State(unit_status=ops.BlockedStatus("waiting for the DB"))
...
assert state_out.unit_status == ops.ActiveStatus()

you would write:

state_in = scenario.State(unit_status=scenario.BlockedStatus("waiting for the DB"))
...
assert state_out.unit_status == scenario.ActiveStatus()

Or, in a future time when the Scenario objects live in ops.testing:

import ops
import ops.testing as testing

state_in = testing.State(unit_status=testing.BlockedStatus("waiting for the DB"))
...
assert state_out.unit_status == testing.ActiveStatus()

In the same way that __post_init__ converts ops status classes to Scenario ones, it will also convert ops Port and Storage objects, and the Scenario classes gain an __eq__ so they can be used more interchangeably with the ops objects. They don't inherit from the ops classes, because isinstance seems unlikely to be useful in these cases, and that would bring in additional baggage.

(I originally planned to remove _EntityStatus in favour of using ops.StatusBase by getting the ops status classes converted to dataclasses, but the @canonical/charm-tech team decided that it would provide a more consistent experience to have the Scenario state only contain Scenario objects (and plain Python ones), and no ops ones.)

@benhoyt
Copy link
Collaborator

benhoyt commented Jun 7, 2024

Thanks, @tonyandrewmeyer. Side note: I think one would usually write import ops.testing as testing as from ops import testing, right?

@PietroPasotti I realize you might disagree with this direction. Happy to hear your feedback and come to consensus (open to a voice discussion about this -- or anything else -- if needed).

README.md Outdated Show resolved Hide resolved
@tonyandrewmeyer
Copy link
Collaborator Author

Side note: I think one would usually write import ops.testing as testing as from ops import testing, right?

Yes one would. It's actually slightly slower (looking with dis, I assume because the latter builds a tuple), but I just generally don't like the aesthetics of mixing "import x" and "from x import y", although it's certainly valid in this case. I can adjust the PR description if you prefer (and I would almost certainly write "from ops import testing" in docs, going for convention over personal preference).

@PietroPasotti I realize you might disagree with this direction. Happy to hear your feedback and come to consensus (open to a voice discussion about this -- or anything else -- if needed).

+1 (but "realise" 😉 ).

@benhoyt
Copy link
Collaborator

benhoyt commented Jun 7, 2024

Haha, you have to reali(z|e) I'm an American Kiwi... :-)

Copy link
Collaborator

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

Not as much against this as I thought I'd be.
I think it's nice to have scenario be more self-contained.

scenario/mocking.py Outdated Show resolved Hide resolved
scenario/state.py Outdated Show resolved Hide resolved
scenario/state.py Outdated Show resolved Hide resolved
scenario/state.py Show resolved Hide resolved
tests/test_e2e/test_status.py Outdated Show resolved Hide resolved
@tonyandrewmeyer tonyandrewmeyer requested review from benhoyt and dimaqq July 9, 2024 10:25
README.md Outdated Show resolved Hide resolved
scenario/state.py Outdated Show resolved Hide resolved
@tonyandrewmeyer tonyandrewmeyer requested a review from benhoyt July 9, 2024 22:09
@tonyandrewmeyer tonyandrewmeyer dismissed PietroPasotti’s stale review July 17, 2024 00:33

Have addressed, Pietro is on PTO, no need to block (can always change before 7.0 release).

@tonyandrewmeyer tonyandrewmeyer merged commit 4209071 into canonical:7.0 Jul 17, 2024
2 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the add-status-classes branch July 17, 2024 03:19
tonyandrewmeyer added a commit that referenced this pull request Sep 2, 2024
Adds classes that match the ops status classes:

* UnknownStatus
* ActiveStatus
* WaitingStatus
* MaintenanceStatus
* BlockedStatus
* ErrorStatus
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.

4 participants