Repository navigation
docs: tidy up the manage resources guide - #2720
Open
tonyandrewmeyer wants to merge 4 commits into
Open
tonyandrewmeyer wants to merge 4 commits into
tonyandrewmeyer wants to merge 4 commits into
Conversation
The example was a module-level function that took self, caught ModelError before NameError even though fetch checks the name first, had a misplaced quote in one of the status messages, opened a Path as though it were a string, and carried a design musing in a comment. Rewrite it as a charm method, say what each of the two exceptions actually means, and use my-resource consistently in the unit-test example so it lines up with the charmcraft.yaml above it.
tonyandrewmeyer
marked this pull request as ready for review
August 31, 2026 04:08
Collaborator
Author
|
@dwilding I've fixed the 'bugs' on the page, but I'm not sure this covers the 'quality' that the issue is asking for. Did you have specific things in mind? Should we be talking more about what you use resources for, or would you know that by the time you get to the how-to guide? I think there's a distinct difference between K8s and machine, should this take that into account more strongly? Might be a good one to talk over in our 1-1? |
The page declares `filename: somefile.txt` for `my-resource` and the test
metadata declared `{'type': 'file'}` with no filename. Scenario doesn't
validate metadata so the test passes either way, but Juju requires a filename
for a file resource, so a reader copying the metadata into a real charm gets a
`charmcraft.yaml` Juju rejects - the same inconsistency this hunk was fixing
in the other direction. The leftover `'name': 'julie'` is now `'my-charm'`.
The snippet spends most of its length on two failure branches and tested
neither. The `NameError` one is a three-line test, so it's shown. The
`ops.ModelError` one cannot be reached from a state transition at all: with
the resource in `meta` and missing from `State.resources`, `ops.testing`
raises `RuntimeError('Inconsistent state: ...')` rather than `ops.ModelError`.
Rather than leave a reader to discover that, the page says so and points at
integration tests.
Both Python snippets on the page were run verbatim to check they work.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A quality pass over the "How to manage resources" guide, which had collected a few things: the example handler was a module-level function that nonetheless took
self, the status message had a misplaced quote (resource 'my-resource; run ... for more info'), there was a comment musing about whether to reraise,logger.error(e)where the rest of the guides uselogger.exception, andopen(resource_path, 'r')on somethingfetch()returns as aPath.I've also swapped the two
exceptblocks soNameErrorcomes first.Resources.fetchchecks the name before it callsresource_get, and its docstring listsNameErrorfirst, so the guide was reading in the opposite order to the implementation. The sentence after the block now says what each exception actually means, rather than the previous single "does not exist" clause that only covered one of them.The unit-test example used a
fooOCI-image resource while thecharmcraft.yamlabove it declares amy-resourcefile resource, so I've made those match. One leftover> See first:blockquote in the integration-tests section is converted too - #2666 swept the file, but #2662 added that section afterwards.Preview.
Fixes #2490