Reviewing charms

How to request a review for charm for being listed on charmhub.io?

First of all, reviewing charms encourages the involvement of the community. The community refers to individuals and organisations creating or contributing to charms, Juju and the Charm SDK. The goals of the review are:

  1. Be transparent about the capabilities and qualities of a charm.

  2. Ensure a common level of quality (publication readiness stage of the charm maturity.

Key considerations

  1. The process is streamlined: The party requesting the review provides structured evidence as input to the review process.
  2. A review is transparent for the community. Review and the review comments are public.
  3. Everyone can participate in the review, for example, participate in a discussion on discourse. A review may benefit from the expertise of a reviewer in the relevant field. Thus, the review process is flexible and open to involving multiple persons.
  4. The review covers the effective automation of tests for automated approvals of subsequent releases.

Steps of a review

The specification provides details and summaries of how the review works. However, the overall approach is straightforward:

  1. The author requests a review for one charm at a time with all prerequisites posted using a form for the public Discourse forum (see link below).
  2. The review works on a Github pull request that covers the whole source code of the charm (or on a merge request on other social coding platforms). Thus, the author prepares a GitHub pull request for code review.
  3. The reviewer checks if the prerequisites are met and the pull request is ready.
  4. The public review is carried out as a conversation on the pull request.
  5. The review concludes if the charm is “publication ready” or even “evaluation-ready”.
  6. If the review is at least “publication-ready”, the store team is asked to list the charm.

The result of the process is that:

  • if the review is successful, the charm is switched to listed mode, or
  • if the review is unsuccessful, the charm does not reach the required criteria and the charm remains unlisted.

Roles and concepts

The charm review process consists of the following elements:

Role or item Description
Forum Refers to the category charmhub requests on discourse.charmhub.io. Using this public channel ensures transparency to the community.
Author Author of the charm or person representing the organisation. The person submitting the charm for review is called the author in this document.
Publisher The responsible person or organisation for publishing the charm.
Review group A group of contact persons watching if review requests arrive and requesting modifications in case or assigning a review to a suitable reviewer. The reviewer group is committed to watching the forum.
Reviewer Person conducting the review.
Listing After the reviewer has reviewed the charm successfully, it can be switched to ‘listing’. Listing means that the charm will be part of the search result when querying the Web pages and API of Charmhub.io.
Without ‘listing’, the charm will be available under its URL but is not listed in searches.
Review criteria The review criteria listed in the Charm SDK documentation under Charm Maturity.

Creation of a review pull request

The review of the charm is prepared by the author, who requests the review, using the discourse form listing all prerequisites. The form also serves as a checklist.

The review of the source code of the charm uses a Github pull request for comments and conversations directly at the source code. Note that the pull request is usually not suitable for carrying code changes resulting from the review comments; this is likely going to happen elsewhere.

The author prepares a review pull request: The approach uses two branches: an empty branch and a new branch containing all the files of the charm. Then, a pull request merging all files into the empty branch will cover all files for discussion.

In detail, the procedure is as follows:

  1. Create an orphan branch using git switch --orphan charm-review-base
  2. Add some random file and commit it. Then, remove the same file in a subsequent commit.
  3. Now this branch is recognised by git and can be pushed to the remote origin GitHub: git push origin charm-review-base - This is the empty branch to merge into.
  4. Create a new branch with charm-review-base as the base: git checkout -b charm-review
  5. Copy all the files from main without the commit history: git checkout main -- . - At this step, all files from the charm are subject for adding into the previously created empty branch: comments can be made at every line of the code of the charm.
  6. Go to GitHub and create a PR from charm-review to charm-review-base.

Here is a script for automating the entire creation:

#!/bin/bash

# Create an orphan branch
git switch --orphan charm-review-base

# Add a random file and commit it
echo "Random content" > random_file.txt
git add random_file.txt
git commit -m "Add random file"

# Remove the same file in subsequent commit
git rm random_file.txt
git commit -m "Remove random file"

# Push the branch to remote origin GitHub
git push origin charm-review-base

# Create a new branch with charm-review-base as the base
git checkout -b charm-review charm-review-base

# Copy files from main without commit history
git checkout main -- .

# Commit and push the files
git add .
git commit -m "Copy files from main"
git push origin charm-review

Please remember that the procedure is only to enable the discussion on a pull request. It is not meant to apply changes to the branch “charm-review” and continue the review. If the review comments result in changes to the charm, the charm is updated and the procedure is repeated for another review cycle.

Review prerequisites

The process has the following prerequisites to be delivered by the author. These prerequisites are also part of the review form which the author shall use for submission:

  1. The charm source code is accessible for reviewers and an URL to a (git) source code repository is available.
  2. URLs to test implementations.
  3. Unit, installation and integration tests are present.
  4. URLs links to test results.
  5. Automation to release to appropriate channels has been implemented.
  6. URLs to usage and contribution documentation is provided,
  7. The publisher of the charm is not anonymous.
  8. The required workload is accessible (if not uploaded to charmhub.io).

In addition, the author creates the pull request on GitHub for allowing for discussions on the charm review.

Review criteria for listing

In summary, the review criteria for listing is about ensuring that it is a good charm. As a reviewer, the following checklist unifies the criteria for each element listed in the definitions:

  • The review item lists the criteria item
  • The objective clarifies what the goal with this point is as an orientation for judging
  • The review criteria columns list how this criteria item could be fulfilled.
Review item Objective Review criteria
Intended functionality Despite all the items for publication readiness, the charm must work. Charm does what it is meant to do - ideally done in a demo.
Charmhub.io charm detail page A complete and consistent appearance of the charm is required for a quality impression of the charm collection. The overall appearance looks good, which means:
* The name complies with the naming guidelines.
* The publisher is identified.
* The links are provided.
* The documentation looks reasonable.
Source repository Generally, the source code for charms must be accessible by the community for transparency and collaboration. It is not entirely mandatory to have the charm published as OSS for review, but the repository must be accessible from the persons working on the review request.
Coding conventions The source code of the charm is accessible in the sense of approachability. Consistent source code style and formatting are also considered a sign of being committed to quality. The implemented checks for coding conventions are reasonable and implemented in the regular CI/CD implementation.
Release automation implementation An implementation for automated releasing to charmhub.io improves the ability to provide updates covering vulnerabilities quickly. Release automation for unstable channels to enable testing when new versions of the charm code or the workload become available.
Unit tests implementation In particular, for the charms review, assuring a reasonable test suite is essential to allow for automated releases in future. The unit tests show relevant coverage. It is a case-dependent review.At the time of review, the test runs successfully.
Unit tests results Availability of test results is mandatory for a working collaborative project. URL to test results from CI/CD automation.
Installation test implemented (could be part of the integration test) In particular, for the charm review, assuring a reasonable test suite is essential to allow for automated releases in future. With this test, for every build, it is ensured that the installation is successful. An implementation for checking the installation is present. The implementation should also check for successful installation as part of the automation, and the workload behaves as expected. At the time of review, the test runs successfully.
Installation test results Availability of test results is mandatory for a working collaborative project. URL to test results from CI/CD automation.
Integration tests implemented In particular for the review of charms, assuring a reasonable test suite is important to allow for automated releases in future. With this test, for every build, it is ensured that the integration with other charms is successful. An implementation for testing the required integrations (if applicable) is present. The implementation should also check for successful integration as part of he automation and the workload behaves as expected.
At the time of review, the test runs successfully.
Integration test results Availability of test results is mandatory for a working collaborative project. URL to test results from CI/CD automation.
Documentation for usage The documentation for using the charm should be separate from the documentation for developing or contributing to the charm. URL to this documentation is present.
Documentation for contributing The documentation for contributing to the charm should be separate from the documentation for developing or using the charm. URL to this documentation is present.
Licensing statement For the charm shared, OSS or not, the licensing terms of the charm are clarified (which also implies an identified authorship of the charm). URL to the ruling licensing statement is present.

Get started

If the charm is ready for review, then the review pull request can be created with the script above and the posting can be made using this link for the review form:

Announce charm ready for review.

2 Likes

One open point not covered so far:

For the form for the discourse post (link at the very bottom of the post), put the links to the file in the Web-repo-view or the file as part of the review-pull request. I would find the review pull request more convenient for the reviewing person. But for the documentation, people might prefer the link to the actual file in the repo.

Also, another change that is coming: Covering only one charm per review request makes a lot of sense!

This is great @mcjaeger!

I just completed a review using this and had a few conclusions:

  • I find the checklist imposes a high cognitive load on the reviewer: I need to unabstract the abstractions.
  • I believe some items in the table could be consolidated.

The checklist above is great for rationale, but I think for actual reviews we could use something more lightweight, wdyt?

For example (usage example):

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.
Source repository Link to source repository accessible by reviewer
Coding conventions A reasonable styleguide is enforced in tox.ini/similar and in CI/CD.
Release automation implementation CI auto releases charm to edge on merge.
Unit tests implementation Unit tests are run in CI, pass, have at least 50% coverage, and their results are available to the reviewer.
Integration tests implemented Integration tests for installation, usage and relations are run in CI, pass, and their results are available to the reviewer.
Documentation for usage A usage doc exists separate from README.md.
Documentation for contributing A contributing doc (e.g. CONTRIBUTING.md) exists.
Licensing statement LICENCE is clearly available to potential users.
1 Like

it is a good comment, the review form with the high cognitive load was just a start - to kick this off and also to have it in a form. I m not sure I understand exactly why it is difficult to fill the first one out. I ll try to update the form link to have an updated form for the usage example.

However, referring to the usage example, I still find it is useful, @sed-i, @gruyaume to have a

  1. one review request per charm
  2. a review pull request, because it feels already messy to follow the comments in Make TLS charms searchable and visible

Once we go through the charm review process, we already have code in main. How do you expect people to go through a pull request? Should we create a dummy branch and go through a “dummy” pull request? I’m not sure that’s a great experience either.

The dummy branch has the advantage how having all the conversations on the code / file contents under one URL. We have also discussed to create an issue and collect review points in that issue. What we do not want to have is individual links to the source tree mentions in different discourse replies. What would be your preference, @gruyaume