refactor: import the ops-scenario repository #1406

tonyandrewmeyer · 2024-10-02T07:08:21Z

Adds the content of the ops-scenario repository (including history) into the repository, for easier long-term maintenance. The previous ops-scenario content is located primarily in the new top-level testing folder (named for ops[testing]), other than some content that has been removed (e.g. separate docs, .pre-commit-config.yaml and so on) or merged (e.g. tox.ini).

There are more than 700 commits because the history is imported, and a lot of new code, so this will be an unusual one to review. I suggest starting with the most recent commits (basically everything after what's in ops-scenario) and then reviewing each commit individually. To be clear: I didn't touch any of the ones before this, they were generated with a git merge --allow-unrelated-histories from a clone of the canonical/ops-scenario repo.

The commits are:

Moving everything other than .git in ops-scenario to ops-scenario/testing
Copying ops-scenario to operator
Delete files that aren't needed any more, and move some others
Adjust .gitignore
Small linting changes to match with operator
Fix issues that our linter found - mostly unused imports, but also a couple of shadowed tests and other small issues
Add test-readme to operator/tox.ini, install ops-scenario for testing via the local copy
Adjust the ops-scenario pyproject.toml
Drop out-of-date parts of ops-scenario pyproject.toml
Remove an unnecessary CI workflow
Partially adjust the CI workflow for publishing ops-scenario - this needs to switch to Trusted Publisher and maybe some other tweaks - it seems best finalised in a separate & dedicated PR
A bunch of fixes so that tox -e static is happy with the ops-scenario code (not tests) - this is mixed quality - there are too many type: ignore and Any uses, but it's reasonable and better than before and gets us to a place where the static checks pass with the same config as for the rest of the code. I feel like further improvements can be done separately (and also ideally type checking the ops-scenario tests, I guess).
Small change to an ops-scenario test to not clash with the ops.charm name
Adjusts the ops-scenario tests to use relative imports for the test_helpers file - I'm not sure this is the best solution, but it seemed ok for now. We actually want to get rid of the trigger helper anyway, so maybe the problem just goes away in time.
Adds a dependency for the unit tests
Adjusts pyproject.toml to handle the repo not being flat-layout any more, and static checks ops-scenario
Adjusts the ops-scenario tests to pass - these are puzzling. I don't understand why pytest.raises does not work for one set of tests (previously shadowed so not running), any advice on that would be great. I'm also not sure why the other tests weren't failing previously, since they were clearly wrong.
Use the ops._private.harness.ActionFailed rather than defining a new one (as discussed/planned previously - this is needed in this PR to avoid clashing in the tests).
Fix consistency-checker looking for items in the state - this is a bug (see consistency checker gives false positives when working with copied objects ops-scenario#199), and I thought it was required to fix one of the tests, but I think actually a different fix was required, but included this since it's needed rather than back it out.
Ensure that we're testing against the local code and not code from PyPI
Skip the weird pytest.raises issue mentioned above for now, rather than working around it
Ignore the first two commits listed here in blame

Note that I have not done the " to ' conversion in this. It seems like that could be a separate and dedicated PR.

There's essentially no code changes, other than addressing issues that the operator tox unit, lint, or static found, just a bunch of moving things around.

Adds support for `collect-status`

fix falsy config defaults

fix secret id canonicalization

…xt.cleanup()

Remove scripts

added container fs temporary dir cleanup in

PietroPasotti

Just a quick look for now, will give some more thoughts later.
Something that comes to mind is: is the idea to do (eventually) from ops.scenario import ...? still on the table, or is the 'scenario' namespace going to remain toplevel?

PietroPasotti · 2024-10-02T07:51:03Z

testing/src/ops_main_mock.py

+try:
+    from ops._main import CHARM_STATE_FILE, _Dispatcher, _get_event_args  # type: ignore
+    from ops._main import logger as ops_logger  # type: ignore
+except ImportError:


the imports here should be simplified now that we know what ops version we're being shipped with

or, maybe not, since it's still an independent package that could be installed alongside older ops. nvm

It's probably true that it could still theoretically be installed alongside older ops, but in that case why wouldn't you also just use an older ops-scenario version? I think part of the reason it's good in a single repo is so we can do atomic commits for things that use _private functions.

However, it might make sense to keep this PR limited to just copying code 1:1, and then subsequent PRs for updates to those files. Will make review saner I think, and mean we're not tempted to include other changes here.

Yeah, I had to bump the minimum ops version in this PR so the compatibility code can be removed, but I would rather do it in another PR, since this one is already complicated to review.

PietroPasotti · 2024-10-02T07:52:13Z

testing/src/ops_main_mock.py

+    return dispatcher, framework, charm
+
+
+class Ops:


perhaps we can do that later, but now is a good time to trim down ops_main_mock and start using directly some of the newer stuff in ops

I think it is good to do that soon, but not in this PR, which is already very large/complex.

tonyandrewmeyer · 2024-10-02T08:06:22Z

Something that comes to mind is: is the idea to do (eventually) from ops.scenario import ...? still on the table, or is the 'scenario' namespace going to remain toplevel?

As of ops 2.17 you can do this:

from ops import testing
ctx = testing.Context(MyCharm)
ctx.run(ctx.on.start(), testing.State())

The intention is to encourage use in that form.

benhoyt · 2024-10-02T21:48:05Z

One thing to note is we'll want to merge with the "Rebase and merge" option (re-enabling that in Settings temporarily).

benhoyt · 2024-10-02T22:12:03Z

Thanks @tonyandrewmeyer, this is excellent work. A few thoughts/questions:

During the import, did you scan through the individual commits to see if there were mistakes like large binary files committed, only to be deleted later? I've been burnt by this before, there was some 100MB+ binary added to the repo history. The main thing is: let's ensure the .git repo size is not too much larger than before (would you be able to provide before/after numbers?).
Yeah, I think we should probably add those renames to the blame-ignore list.
We can probably leave the puzzling pytest.raises thing for a separate PR just to keep this limited (TODO and commented-out code is fine for now).
How do the imports like from scenario.errors import NoObserverError in testing/src/runtime.py work? In other words, where is that scenario name coming from? Should these be from .errors import Foo and the like? Or maybe not -- but I just don't understand how this is working as is.

tonyandrewmeyer · 2024-10-02T22:32:43Z

During the import, did you scan through the individual commits to see if there were mistakes like large binary files committed, only to be deleted later?

No. I briefly scanned through the commits (I was originally thinking of ones that said "accidentally committed password") but only the title and that's not really going to tell the whole story. I could read through the details of all the commits, but that would take quite a while.

The main thing is: let's ensure the .git repo size is not too much larger than before (would you be able to provide before/after numbers?).

A fresh ops-scenario clone is 2.6M (.git is 1.8M). operator/.git is 3.2M main@HEAD. operator/.git is 4.8M when cloned with --single-branch --branch=import-ops-scenario from my fork. So it seems ok - it is admittedly a ~50% increase in size, but my guess is that the only way around that would be to start squashing things, which seems very complex and time-consuming.

Yeah, I think we should probably add those renames to the blame-ignore list.

Since this will be a rebase-and-merge, does that mean I can do that now rather than in a follow-up PR because the hashes won't change?

We can probably leave the puzzling pytest.raises thing for a separate PR just to keep this limited (TODO and commented-out code is fine for now).

Ok, I can change that.

How do the imports like from scenario.errors import NoObserverError in testing/src/runtime.py work? In other words, where is that scenario name coming from? Should these be from .errors import Foo and the like? Or maybe not -- but I just don't understand how this is working as is.

The name comes from the testing/pyproject.toml configuration. If you do a pip install . inside of testing, you'll get a package called scenario installed.
They probably should be relative imports, but I felt like that could wait for a follow-up PR.
Doing pip install .[testing] at the top level might not be doing what we want for the tests - I think maybe it's pulling ops-scenario from PyPI so testing against a mixture of the latest release (when importing from scenario) and the local code (everything else). It probably needs to be pip install . done in both places instead, maybe with a specific test for pip install ops[testing]. I hadn't thought about this - I'll look into it.

benhoyt · 2024-10-02T22:41:59Z

Thanks!

I think 50% bigger is totally reasonable. I was thinking of 10x or worse blow-up. I don't think we need to do anything further here. If there were passwords, they'd been in the public ops-scenario repo already. :-)

Since this will be a rebase-and-merge, does that mean I can do that now rather than in a follow-up PR because the hashes won't change?

Yeah, that sounds right.

The name comes from the testing/pyproject.toml configuration. If you do a pip install . inside of testing, you'll get a package called scenario installed. They probably should be relative imports, but I felt like that could wait for a follow-up PR.

Great, that sounds fine to me.

Doing pip install .[testing] at the top level might not be doing what we want for the tests - I think maybe it's pulling ops-scenario from PyPI so testing against a mixture of the latest release (when importing from scenario) and the local code (everything else). It probably needs to be pip install . done in both places instead, maybe with a specific test for pip install ops[testing]. I hadn't thought about this - I'll look into it.

👍

james-garner-canonical · 2024-10-02T23:09:05Z

testing/README.md

@@ -304,7 +304,7 @@ def test_relation_data():
        ),
    }

-# which is very idiomatic and superbly explicit. Noice.
+# which is very idiomatic and superbly explicit.


This is funny

It made me sad to remove it, but codespell complained and it seemed silly to keep it and put in a codespell bypass just for this.

hahaha I think I added that during an early live demo, too bad it has to go :)

testing/src/context.py

james-garner-canonical · 2024-10-03T00:03:50Z

Thank you for a very clean series of commits, @tonyandrewmeyer.

The consistency checker fix seems simple enough to me that it would be fine to solve it in this PR even if it's not strictly needed to get everything that's currently working in scenario working here, especially since it's the last commit, so doesn't interfere with understanding the others.

More for my own understanding than anything, are these all the follow up issues coming out of this PR?

finish adjusting the CI workflow for publishing ops-scenario - this needs to switch to Trusted Publisher and maybe some other tweaks -- high priority, as this is needed before next release?
investigate puzzling pytest raises issue -- lower priority, since the test will be disabled for now
type check the ops-scenario tests and reduce the use of Any and # type: ignore in ops-scenario -- lower priority, nice to have
edit: from daily sync -- archive old repo: update readme, migrate issues, figure out what to do with PRs

edit: These will all be in Jira under the epic for this.

I'll wait to hear back on the situation with tox potentially installing scenario from PyPI in testing. I wasn't clear on explanation of the scenario imports with regards to this, should they work if scenario isn't being installed from PyPI, or should they become relative imports?

tonyandrewmeyer · 2024-10-03T01:21:14Z

[Ben]

If there were passwords, they'd been in the public ops-scenario repo already. :-)

Yes, and if there was a "get rid of the password I committed earlier" commit I assume they would have been invalidated too. But security scanners tend to look through a repo's entire history and in my experience it's a pain to have to explain that a problem was already solved.

[James]

More for my own understanding than anything, are these all the follow up issues coming out of this PR?

finish adjusting the CI workflow for publishing ops-scenario - this needs to switch to Trusted Publisher and maybe some other tweaks -- high priority, as this is needed before next release?

Yes. It could be made to work by just adjusting the working directory and putting an appropriate secret into our repo, but I would rather go straight to Trusted Publisher and we also need to decide if ops-scenario will still publish automatically whenever the version is increased or if it moves to a manual process like ops.

investigate puzzling pytest raises issue -- lower priority, since the test will be disabled for now

Yes. I am super baffled by this. I'm hoping that if I come back to it in a few days it'll be really obvious.

type check the ops-scenario tests and reduce the use of Any and # type: ignore in ops-scenario -- lower priority, nice to have

Yes. I suspect there's a bunch of type annotation improvements that could be made in general - I don't think that was a focus for ops-scenario in general.

I'll wait to hear back on the situation with tox potentially installing scenario from PyPI in testing. I wasn't clear on explanation of the scenario imports with regards to this, should they work if scenario isn't being installed from PyPI, or should they become relative imports?

I believe if I do the pip install . for both packages then they don't need to be relative, because everything will be installed just like as if from PyPI. But probably relative imports are the more correct way to go in general? We don't do that in ops either, though.

I think that is everything that directly arises from this PR. There are some other things that should be done (e.g. using the _Manager and _JujuContext classes that ops recently added, removing the compatibility for older versions of ops, but I think they all have separate tickets.

james-garner-canonical · 2024-10-03T01:35:25Z

Thanks, @tonyandrewmeyer. I don't have a strong opinion on relative imports, just wondering about the resolution for this:

I think maybe it's pulling ops-scenario from PyPI so testing against a mixture of the latest release (when importing from scenario) and the local code (everything else).

If this is working correctly with scenario imports, then all good. Happy to approve if/when this is the case in current tests (is it already?).

benhoyt

Thanks for your work on this. Let's get final approval from @PietroPasotti as well.

tonyandrewmeyer · 2024-10-03T02:47:02Z

If this is working correctly with scenario imports, then all good. Happy to approve if/when this is the case in current tests (is it already?).

It is the case now - I think it was mixing local and PyPI previously (a few small extra things to fix showed up afterwards also). It's not perfect now, because of the way that Tox doesn't know to regenerate an environment if it's a local install but it will work correctly and is ok for now I think.

tox.ini

PietroPasotti · 2024-10-09T06:58:56Z

testing/src/_consistency_checker.py

-    elif storage not in state.storages:
+    elif (storage.name, storage.index) not in {
+        (s.name, s.index) for s in state.storages
+    }:
        errors.append(
            f"cannot emit {event.name} because storage {storage.name} "


we might want to mention the storage.index as well in this error message too since we're basing our check on that

Good point. I'll do a follow-up PR to do that and add tests for this case, rather than making this one even larger.

PietroPasotti · 2024-10-09T07:00:26Z

testing/src/_consistency_checker.py

@@ -476,7 +478,7 @@ def check_secrets_consistency(
        return Results(errors, [])

    assert event.secret is not None
-    if event.secret not in state.secrets:
+    if event.secret.id not in {s.id for s in state.secrets}:
        secret_key = event.secret.id if event.secret.id else event.secret.label


if there is a chance that secret.id could be None (seems implied by this line) then we have to be careful about putting them in a set comprehension? or perhaps this line is old code that can now be removed

I'll check on this, but I believe the event's secret should always have an ID and we seem to type it that way. Could indeed be older code.

PietroPasotti · 2024-10-09T07:05:16Z

testing/README.md

@@ -304,7 +304,7 @@ def test_relation_data():
        ),
    }

-# which is very idiomatic and superbly explicit. Noice.
+# which is very idiomatic and superbly explicit.


hahaha I think I added that during an early live demo, too bad it has to go :)

PietroPasotti

Overall it looks great! Thanks for this humongous work :)

tonyandrewmeyer · 2024-10-09T07:55:34Z

@benhoyt would you be able to merge this please? You need to enable rebase & merge, do it, then turn that off again, for this special-case. Thanks!

benhoyt · 2024-10-09T21:59:44Z

@tonyandrewmeyer I've enabled Rebase and Merge temporarily -- go ahead and rebase+merge this one (I'll let you do the honours!), and then I'll disable that setting again.

PietroPasotti added 30 commits November 16, 2023 10:50

Merge pull request canonical#78 from canonical/collect-status-support

70bb022

Adds support for `collect-status`

Merge remote-tracking branch 'origin/main' into remove-scripts

f149b86

vbump

a3dcd2c

snapshot warning

4eac4a5

fix falsy config defaults

1c0ff40

Merge pull request canonical#80 from canonical/fix-falsy-config-defaults

f5ac9c8

fix falsy config defaults

fix secret id canonicalization

89bc81a

vbump

806a4f4

Merge remote-tracking branch 'origin/main' into fix-secret-id-canonical

a6d8efe

vbump

e0019f9

Merge pull request canonical#82 from canonical/fix-secret-id-canonical

31ab9dd

fix secret id canonicalization

added container fs temporary dir cleanup in Context.clear() --> Conte…

ae4a8ee

…xt.cleanup()

vbump

b13405b

Merge remote-tracking branch 'origin/main' into remove-scripts

b2fd95d

Merge pull request canonical#77 from canonical/remove-scripts

7a4e2cb

Remove scripts

uniformed owner 'application'-->'app'

a8bb04d

lint

43e2b12

factored in ownership

33cf4ad

some more tests and metadata rule fixes

0aab2af

simplified access control logic

9633380

Merge branch 'main' into cleanup-container-fs

811e8ea

Merge pull request canonical#85 from canonical/cleanup-container-fs

2789c0a

added container fs temporary dir cleanup in

stripped 'granted'

25935db

Doc fixes

0b89543

BC

9100d1b

extra bindings and network model rework

0dea321

tolerate dead relations

f76cc74

re-rework

d02b8bb

pyright precommit

9cd044f

readme

9928aa9

PietroPasotti reviewed Oct 2, 2024

View reviewed changes

james-garner-canonical reviewed Oct 2, 2024

View reviewed changes

testing/src/context.py Show resolved Hide resolved

benhoyt approved these changes Oct 3, 2024

View reviewed changes

tonyandrewmeyer requested review from james-garner-canonical and PietroPasotti October 3, 2024 02:47

james-garner-canonical reviewed Oct 3, 2024

View reviewed changes

tox.ini Show resolved Hide resolved

tonyandrewmeyer requested a review from james-garner-canonical October 3, 2024 02:54

james-garner-canonical approved these changes Oct 3, 2024

View reviewed changes

PietroPasotti reviewed Oct 9, 2024

View reviewed changes

PietroPasotti approved these changes Oct 9, 2024

View reviewed changes

tonyandrewmeyer added 5 commits October 10, 2024 12:54

Merge remote-tracking branch 'source/main' into import-ops-scenario

4535397

Reorganise the scenario files.

645daed

Tweaks to the scenario code to adjust to the operator repo.

d2f3b51

Adjustments to the scenario tooling for being in the operator repo.

0804fcc

Update the merge/move commits after rebasing.

51ba92f

tonyandrewmeyer force-pushed the import-ops-scenario branch from e41c3ad to 51ba92f Compare October 10, 2024 00:10

tonyandrewmeyer merged commit 51ba92f into canonical:main Oct 10, 2024
30 checks passed

tonyandrewmeyer deleted the import-ops-scenario branch October 10, 2024 01:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: import the ops-scenario repository #1406

refactor: import the ops-scenario repository #1406

tonyandrewmeyer commented Oct 2, 2024 •

edited

Loading

PietroPasotti left a comment

PietroPasotti Oct 2, 2024

PietroPasotti Oct 2, 2024

benhoyt Oct 2, 2024

tonyandrewmeyer Oct 2, 2024

PietroPasotti Oct 2, 2024

tonyandrewmeyer Oct 2, 2024

tonyandrewmeyer commented Oct 2, 2024

benhoyt commented Oct 2, 2024

benhoyt commented Oct 2, 2024

tonyandrewmeyer commented Oct 2, 2024

benhoyt commented Oct 2, 2024

james-garner-canonical Oct 2, 2024

tonyandrewmeyer Oct 2, 2024

PietroPasotti Oct 9, 2024

james-garner-canonical commented Oct 3, 2024 •

edited

Loading

tonyandrewmeyer commented Oct 3, 2024

james-garner-canonical commented Oct 3, 2024

benhoyt left a comment

tonyandrewmeyer commented Oct 3, 2024

PietroPasotti Oct 9, 2024

tonyandrewmeyer Oct 9, 2024

PietroPasotti Oct 9, 2024

tonyandrewmeyer Oct 9, 2024

PietroPasotti Oct 9, 2024

PietroPasotti left a comment

tonyandrewmeyer commented Oct 9, 2024

benhoyt commented Oct 9, 2024

refactor: import the ops-scenario repository #1406

refactor: import the ops-scenario repository #1406

Conversation

tonyandrewmeyer commented Oct 2, 2024 • edited Loading

PietroPasotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyandrewmeyer commented Oct 2, 2024

benhoyt commented Oct 2, 2024

benhoyt commented Oct 2, 2024

tonyandrewmeyer commented Oct 2, 2024

benhoyt commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-garner-canonical commented Oct 3, 2024 • edited Loading

tonyandrewmeyer commented Oct 3, 2024

james-garner-canonical commented Oct 3, 2024

benhoyt left a comment

Choose a reason for hiding this comment

tonyandrewmeyer commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PietroPasotti left a comment

Choose a reason for hiding this comment

tonyandrewmeyer commented Oct 9, 2024

benhoyt commented Oct 9, 2024

tonyandrewmeyer commented Oct 2, 2024 •

edited

Loading

james-garner-canonical commented Oct 3, 2024 •

edited

Loading