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

docs: update how-to guides to use the new unit testing framework #1507

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Dec 18, 2024

Adjustments to the how-to guides:

  • Links to the API reference are updated to the new location. I assume that these can be references that will automatically resolve somehow, but I'm not sure how to do that, so I opened Make internal doc links references #1506 and just fixed them as full links for now (at least they'll work again).
  • A few minor whitespace cleanups.
  • Removed all the Harness sections, moving the Scenario sections up to be "unit tests"
  • Updated the Scenario sections to use ops.testing and Scenario 7
  • Updated the secrets how-to to be more explicit about needing to remove revisions if you create new ones.

Live preview

@tonyandrewmeyer
Copy link
Contributor Author

I weirdly get this locally when I build:

/home/tameyer/code/operator/ops/charm.py:docstring of ops.CharmBase.charm_dir:1: WARNING: py:class reference target not found: pathlib._local.Path [ref.class]
[...]
/home/tameyer/code/operator/ops/_private/harness.py:docstring of ops._private.harness.Harness.get_filesystem_root:1: WARNING: py:class reference target not found: pathlib._local.Path [ref.class]

It doesn't seem like any changes to the how-to guides should cause that. I'll investigate further.

@tonyandrewmeyer
Copy link
Contributor Author

I also get it in main, and in Dora's branch that was just merged in with all the new docs. But I'm sure I re-built the docs in that branch before approving the review, and the rtd CI didn't fail. 😕

@tonyandrewmeyer
Copy link
Contributor Author

Hmm, maybe @dimaqq already found this? sphinx-doc/sphinx#12715

@tonyandrewmeyer
Copy link
Contributor Author

Hmm, maybe @dimaqq already found this? sphinx-doc/sphinx#12715

Yes, running tox with an older Python 'fixes' it.

@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review December 18, 2024 06:46
@tonyandrewmeyer
Copy link
Contributor Author

@tmihoc feel free to review if you have time, but no worries if not :)

Copy link
Member

@tmihoc tmihoc left a comment

Choose a reason for hiding this comment

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

This is massive, thanks, Tony!

PS Thanks also for updating the ops reference links and touching up the language in a lot of other places.

Let of a couple of teeny-tiny comments -- the one at the top about the path for the scenario testing doc is maybe the most relevant at this time.

docs/howto/index.md Show resolved Hide resolved
@@ -108,22 +105,24 @@ def _on_snapshot(self, event: ops.ActionEvent):
...
```

> See more: [`ops.ActionEvent.fail`](https://ops.readthedocs.io/en/latest/#ops.ActionEvent.fail)
> See more: [`ops.ActionEvent.fail`](https://ops.readthedocs.io/en/latest/reference/ops.html#ops.ActionEvent.fail)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for updating the reference links! PS Some day I think we should switch to relative paths everywhere. That way moving those reference docs around will no longer break things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some day I think we should switch to relative paths everywhere. That way moving those reference docs around will no longer break things.

Agreed. I think we can do better than just relative paths and actually use references, but I'm not sure how to do that in the combination of ReST and myst-markdown. So I opened #1506 so that it's someone else's (maybe future-Tony, maybe not) problem 😆 .

docs/howto/manage-configurations.md Show resolved Hide resolved
docs/howto/manage-relations.md Show resolved Hide resolved
docs/howto/manage-secrets.md Show resolved Hide resolved
Copy link
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

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

Look good, just a couple of minor (non-blocking) comments

docs/howto/manage-libraries.md Outdated Show resolved Hide resolved
docs/howto/manage-libraries.md Outdated Show resolved Hide resolved
docs/howto/manage-secrets.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dwilding dwilding left a comment

Choose a reason for hiding this comment

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

Thanks for doing all of this! - No extra comments from me

@tonyandrewmeyer tonyandrewmeyer merged commit dea45c1 into canonical:main Dec 19, 2024
32 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the update-howto-scenario branch December 19, 2024 04:12
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