Livepatch machine charm review #21
Conversation
| on: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
todo: If I am not mistaken, the charm publishing only happens to edge manually, is that right?
We should be aiming to publish/release with every merge to main:
on:
push:
branches:
- main
There was a problem hiding this comment.
Done. I still kept the manual trigger in case we needed them.
There was a problem hiding this comment.
question: Is this meant to be empty?
There was a problem hiding this comment.
Removed, because the relations are already tested in the integration tests.
There was a problem hiding this comment.
question: Is this meant to be empty?
There was a problem hiding this comment.
Removed, because the the action is already tested in the integration tests.
There was a problem hiding this comment.
todo: Code coverage of core charm logic is fairly slim (<40%). There are a lot of conditional paths that aren't tested at all, as well as expected life-cycle events beyond just deploying.
I would aim to cover these as much as possible
There was a problem hiding this comment.
I've improved the test coverage to 86% overall.
Name Stmts Miss Branch BrPart Cover Missing
----------------------------------------------------------------------------
src/actions/enable.py 28 1 8 1 94% 14
src/actions/restart.py 22 1 6 1 93% 14
src/actions/schema_upgrade.py 22 1 6 2 89% 16, 22->exit
src/actions/set_basic_users.py 54 4 18 3 90% 15, 65, 94-95
src/charm.py 198 28 72 18 81% 108-109, 151, 163->166, 168, 173-175, 184-186, 191, 199->exit, 236-237, 248, 251-252, 267, 282, 303, 372-373, 378-379, 382-389, 401, 410
src/constants/errors.py 2 0 0 0 100%
src/constants/snap.py 5 0 0 0 100%
src/constants/statuses.py 6 0 0 0 100%
src/state.py 32 0 4 0 100%
src/util/schema_tool.py 19 4 0 0 79% 22-23, 37-38
----------------------------------------------------------------------------
TOTAL 388 39 114 25 86%
There was a problem hiding this comment.
todo: This is fairly bare bones I think for a published charm. I think it should be expanded on for a wider audience.
I've found that the canonical/seldon-core-operator README is a great example
There was a problem hiding this comment.
Done. The readme files (charm and bundle) are now more fleshed out.
| @@ -0,0 +1,22 @@ | |||
| # Canonical livepatch machine charm | |||
There was a problem hiding this comment.
nitpick/style: Consider captialising 'Livepatch' throughout. It looks better imo.
There was a problem hiding this comment.
nitpick/style: Consider adding the following at the top of the repo:
[](https://charmhub.io/livepatch)
[](https://github.com/canonical/livepatch-machine-charm/actions/workflows/release.yaml)
[](https://github.com/canonical/livepatch-machine-charm/actions/workflows/ci.yaml?query=branch%3Amain)
[](https://github.com/canonical/livepatch-machine-charm/actions/workflows/sync_docs.yaml)NOTE - The links probably aren't correct, but hopefully they are self-explanatory enough to be replaced by ones relevant to your charm.
There was a problem hiding this comment.
nitpick/style: Depending on the licensing and ownership of the product, you may be allowed to add logos and branding images. This goes a long way in legitimising the codebase.
There was a problem hiding this comment.
suggestion: You can also link the Tutorial/How-To docs by following this https://juju.is/docs/sdk/add-docs-to-your-charmhub-page
Or by asking @tmihoc to give better instructions than I can provide. The end result will be something like:
There was a problem hiding this comment.
I fixed the casing you mentioned, and also added the badges that apply. But for the logo and the organized docs, we don't have them yet, and as this is an optional thing, I think we'd like to proceed without them for now.
There was a problem hiding this comment.
todo: Similar comments here as the base README. Though this can be focused more specifically on the features of the bundle itself, rather than the product as a whole.
|
@marcoppenheimer As I've already told you in the chat, here's the updated PR for review: #34 |
No description provided.