we would like the Kratos External IDP charm belonging to the Identity Platform to be searchable /visible in Charmub.
Can you please help us to have it reviewed?
Thanks @shipperizer for the review request! @tony-meyer can you please help with this review? You can go through and tick off the items on the checklist available here and post the result in this thread. This prior listing request could be an example of how to go about it.
I don’t see any PR to review as per the review instructions. However, the charm and lib code is not large, so I just read through it outside of a review and made comments in the checklist below. For the “does what it is meant to do” a demo (as suggested) is preferable for a charm like this that integrates with external providers, but I’ve done a limited test.
| Checked | Review item | Objective | Review criteria | Reviewer Notes |
|---|---|---|---|---|
| ✗ | 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. | The README has instructions for running actions that don’t work (in Juju 3.x), run-action should be run (was run-action a Juju CLI command in the past?), and there is no enable or disable action defined. |
| ✓ | 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. |
See also documentation notes below. |
| ✓ | 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. | codespell, flake8, isort, black (no static type checking, which is fine, just a note). Generally the Charm Tech team recommend doing “import ops” and “ops.X” rather than “from ops.Y import X” (you don’t have to do this, just mentioning it as an FYI). jsonschema should be in PYDEPS |
| ✓ | 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. | Ideally, this commented line wouldn’t be left in the test code. Unclear why the action tests (e.g.) call the charm private method instead of having Harness trigger the action. |
| ✓ | 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. | build & deploy integration test covers this. |
| ✓ | 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 the automation and the workload behaves as expected. At the time of review, the test runs successfully. |
The only integration test is one that builds, deploys, and waits for an initial status - there isn’t any testing of the charm/lib or integration with other charms. Also checked charm-relation-interfaces and there are none there. |
| ✓ | 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. | It would be nice if “microsoft providers” and “apple providers” were properly capitalised in the config descriptions. There’s an “and” missing in the paragraph describing the lib and “the the”. |
| ✗ | 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. | The deploy example at the end provides http-bin as a resource, which does not seem to be required. |
| ✓ | 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. |
hi @tony-meyer
we addressed some of the concerns in https://github.com/canonical/kratos-external-idp-integrator/pull/64
the charm is meant to be used in tandem with the kratos charm, it basically meaningless on its own, hence majority of integration resides in https://github.com/canonical/iam-bundle/blob/main/tests/integration/test_bundle.py
we are not planning to address the issue further as we do believe that covers enough
regarding the tutorial we have https://charmhub.io/topics/canonical-identity-platform/tutorials/e2e-tutorial that requires quite a lot more than simply the charm, happy to refer to it somewhere if needed
let me know
Thanks! Adjustments:
| Checked | Review item | Objective | Review criteria | Reviewer Notes |
|---|---|---|---|---|
| ✗ | 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. | run-action fixed, but there’s still reference to enable and disable actions that don’t exist (looking at the code, I believe these are meant to be setting config) |
| ✓ | 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. | jsonschema now in PYDEPS |
| ✓ | 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. | Looking good! |
| ✓ | 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 the automation and the workload behaves as expected. At the time of review, the test runs successfully. |
Good tests are in the newly provided location |
| ✓ | 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. | Minor issues fixed. |
| ✓ | 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. | Deploy example fixed. |
The tutorial doc is really good - it seems like it would be worth linking to it, or maybe to the identity home from the charmhub page. It’s probably wrong for it to be the “docs” link, since it’s only one piece of the identity platform, but maybe just a freeform link in the charm description?
Speaking of links, adding the issue & homepage links into metadata.yaml (or charmcraft.yaml) would be nice, although I don’t think it’s required for listing.
If the doc’s run <unit-name> enable is fixed to be config <app-name> enabled=false (and similar for disable), and optionally add those extra links, then everything else looks good to me 
.
Hey @tony-meyer ,
Thanks for the feedback. The README has been fixed (there was also a bug there which was fixed, so thanks for that as well).
Also the links have been added (instead of pointing to the bundle, it is now pointing to the topic page).
I think it should be fine now.
Thanks for the review @tony-meyer! The identity team is looking into improving the documentation for each charm based on the feedback. Once done they’ll post an update on this request.
Hi, our other charms have passed the review so I suppose our docs are now good enough. Can we move this forward?
@nsklikas I don’t see the updated documentation on charmhub for this charm yet. Can you please check and add Tutorials, How To, and Reference sections where needed?
Hi,
The charm should now be listed.
Thanks,
Deep
