Review `vault-k8s` for Listing

charmhub requests review requests
1

Hello team! Could you please review vault-k8s for listing on charmhub? This is a k8s charm for HashiCorp Vault and it is developed and maitained by the identity-credentials team.

Metadata links

CI Links

Documentation Links

1 Like
2

Thanks @yazansalti for the review request!

@arturo-seijas can you please help with this review? You can go through and tick off the items on the checklist and post the result in this thread. This prior listing request could serve as an example of how to go about it. Please ping @review-coordinators for any questions.

3

@arturo-seijas @tony-meyer could we get this listing reviewed?

Thank you and let us know if anything is missing,

4

I’ve pinged Arturo on MM as well in case they’re not getting notifications here.

5

I was looking at the review comments and is this the intended way to do charm listing review?

It’s not so much that I don’t agree with the comments, they are mostly valid. But it’s also clear that most of them don’t match with the review checklist and existing listed charms would not satisfy.

6

The focus should be on the attributes listed in the Reviewing charms post, specifically in the review criteria section (which is essentially the same as the corresponding section in the spec).

I think that we do have an issue at the moment that sometimes there is more of a code review than focusing on the criteria we have defined for listing (maybe a code review should be added, and certainly it can have valid as you imply, but it’s indeed not really in the current list, other than “coding conventions”. I had a stretch item on this cycle’s roadmap to try to make the review process clearer and smoother, but it’s looking unlikely that I’ll be able to get to that before The Hague - I’ll try to make sure that Charm-Tech gets to it next cycle.

@arturo-seijas: as @gruyaume said, the code review is helpful, but could you please focus on the review criteria? The easiest way to do this is to copy the checklist into a comment here and tick off (or not!) each item as you verify it. Thanks!

1 Like
7

This is the provisional checklist. Some items are still pending

Checked 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] https://juju.is/docs/sdk/naming). * 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.
1 Like
8

Hi @tony-meyer , I’m addressing everything as a whole. As part of the PR review a couple of points that might compromise some of the review criteria have been raised. I’m posting the current checklist and will update once those items are clarified so that you can get a sense of the current status.

1 Like
9

Hello, @arturo-seijas @tony-meyer

The PR that addressed your review comments was merged:

Is there anything else we need to do to get the charm listed?

Cheers,

10

Hi @gruyaume , it isn’t as far as I’m concerned. I’ve already updated the checklist to reflect the last changes

11

Sorry, I didn’t realise that the list had been updated. Thanks @gruyaume and @arturo-seijas !

@odysseus-k This one should be good for listing now, thanks!

12

Hi @odysseus-k Do you have any updates on this?

Thanks

13

Hi,

@odysseus-k is on leave at the moment. But I believe that the vault-k8s charm has been listed.

Thanks,

Deep

14

It does not show up when we search for vault or vault-k8s. I don’t think it’s been listed.

15

@gruyaume Ah you’re right. The reason the charm is not showing up on Charmhub.io is because the release that the charm is defaulted to is the latest/edge release. Charms will not show up unless they are defaulted to a channel that has a stable release.

Your two options would be to either change the default track of vault-k8s (which I can do), or to release a revision to latest/stable.

Please let me know what you (and/or @yazansalti) would like to do.

Thanks,

Deep

16

Thanks for looking into it @superalpaca.

I think we should do both. The charm will be released to latest/stable soon.

What do you think the default track should be @gruyaume ? I think it should either be 1.16/stable or latest/stable.

17

Could you please change the default track to 1.16/stable? @superalpaca

18

I’ve now set the default track to 1.16 for vault-k8s.

It’s possible that it won’t show up as listed just yet, and that you’ll have to release a revision to 1.16/stable for it to be shown. Let me know if there’s any further issues.

19

Yes I can find it now when I search for vault-k8.

Thank you :slight_smile:

1 Like