Review `Livepatch` machine charm for listing

charmhub requests review requests
1

PR for code review: https://github.com/canonical/livepatch-machine-charm/pull/21

Metadata links

  • Publisher: Canonical Commercial Systems
Review item Review criteria Evidence / notes
Charmhub.io charm detail page The overall charmhub.io appearance looks good, which means: * The name complies with the naming guidelines. * The publisher is identified. * The links are provided. * The documentation looks reasonable. Livepatch on Charmhub
Source repository Link to source repository accessible by reviewer repo
Coding conventions A reasonable styleguide is enforced in tox.ini/similar and in CI/CD. tox file + the operator workflow unit tests runs code linting, language, format, and other checks
Release automation implementation CI auto releases charm to edge on merge. test workflow and the publish workflow
Unit tests implementation Unit tests are run in CI, pass, have at least 50% coverage, and their results are available to the reviewer. using the operator-workflow test and the unit tests and the results
Integration tests implemented Integration tests for installation, usage and relations are run in CI, pass, and their results are available to the reviewer. Integration test results. Also The github action is here which runs tests from this directory
Documentation for usage A usage doc exists separate from README.md. Livepatch docs and discourse tutorial
Documentation for contributing A contributing doc (e.g. CONTRIBUTING.md) exists. Contribution guide
Licensing statement LICENCE is clearly available to potential users. License info
  • There are other actions in the repo to publish the bundle, promote the charm, publish the charm, etc.
2

Hello, @0x12b , @varshigupta would you know a colleague who would have time to take over the review of this charm?

3

Hi @marcoppenheimer,

Would you mind assisting @mina146 with a charm review using the checklist posted here?

Thanks, Simon

4

Hello @mina146, in case you were out for a longer break, please do not forget to consider to use the checklist Simon has proposed.

5

Hi @mcjaeger please find the post updated with the requested checklist

6

hi @mcjaeger any updates on this please? We want to start another charm review for our k8s charm, but we are waiting to see what would be the feedback on this one to resolve comments on both together as they are very similar.

7

I am sorry, I did not notice that you have used the form already to update the initial post, my bad. I was confusing things. To correct:

  1. The reviewing person needs to use the form linked at the end of the review process description:

Review request form

  1. The reviewer would use the form that you have provided.

  2. Marc would be the candidate for reviewing the charm.

For now, maybe the input you have provided is sufficient, but the next charm, pls consider using the “Review Request Form”

8

Hi @marcoppenheimer, maybe there was not much time after your break, but did you have a chance to have a look at reviewing this charm so far?

9

Hello @mina146, the Publisher name is not ideal for any external persons to understand who “Yellow Squad” is? Who would be behind it? I know that taking a look on launchpad is possible, but at a first glance, the publishing org is not clear.

If that is maintained by us, we would need to have a publisher name indicating “Canonical”. How about “Canonical Commercial Systems” as a publishing group? Or, how would commercial systems like to appear in the public as publishing group?

10

Hi @mcjaeger please find the ownership transfer request here:

11

many thanks for following this up - do I understand this correctly, the display name would be “Canonical Commercial Systems”?

12

Have left my review in Github.

Main focus-points:

  • Increasing unit-test coverage
  • README and general documentation needs more detail
  • Ensuring that publish CI triggers on merge to main
13

Hello @mina146, for having a bit more status on the thread, could you please let us know if the comments by Marc do make sense to you?

14

(@marcoppenheimer Sorry for the mention, but, unfortunately, neither @mcjaeger nor @mina146 are working here anymore.)

I’ve applied your suggestions and I’m going to ask for another review. But I’m not sure about the procedure. Should I just open another PR and ask for review here (by this very comment) or should I post a separate forum post?

One thing to add is that our charm is already public/listed and this review process is to make sure it meets the company standards.

15

After the changes in this PR I am happy to approve this! :slight_smile:. Obviously when the int-tests pass, but I presume that’s being worked on. The code itself is much better.

16

@marcoppenheimer The integration tests were failing due to some issue with the runners. I retried the job this morning, and it’s passed.

17

Since the charm has already been listed, this review request is complete.