Thanks @arturo-seijas for the review request! @gmerold 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 serve as an example of how to go about it. Please let me or anyone from the @review-coordinators know if you have any questions.
hello,
if the charm is maintained by Canonical IS, and is not a personal project by you @arturo-seijas, please change the ownership of the charm (with a post for charmhub requests and asking for changing the ownership)
Apologies for a late response. I completely missed this request.
Please find some comments below:
charm_state.py
1. Do new need `KNOWN_CHARM_CONFIG` in the charm_state.py? Can't we just validate whatever config is given against the model?
2. Not sure about the name `CharmState`. To me, it looks more like a CharmConfigValidator ;)
charm.py
1. Do we need to handle smtp and smtp_legacy separately?
2. Not sure about using `saml` in the context of the relations. I think it's a bit confusing. I'd stick to SMTP.
renovate.json
1. Any reason why do we mention `rockcraft.yaml`? There's a lot of unnecessary stuff there. Please review the file.
LIB:
1. In the main docstring, Provider Charm section, should the example also show how to use `update_relation_data`?
2. In the main docstring, Provider Charm section, provider charm doesn't need to import `SmtpDataAvailableEvent`.
3. In the main docstring, imports are missing `smtp` after `charms.smtp_integrator.v0`. `self.smtp` declaration from line 22 also needs fixing.
4. Not sure about the `relations` property in `SmtpProvides`. IMO relations belong to the charm not the library. I'd move it to the charm.
5. IMO warnings about insecure auth_type and transport_security come far too late. How about placing them together with config validation?
6. In `SmtpRequires`, if relation data validation fails, we don't get any feedback.
7. Do we need `get_relation_data` in `SmtpRequires`? Shouldn't the `smtp_data_available` event be used as a public interface here?
TESTS:
In general, it feels like the docstrings could provide a bit more context about what's exactly happening in the tests.
Nuances of smtp and smtp legacy can be really confusing; when saying `invalid data`, it would be good to explain why/how the data is invalid.
1. test_charm.py
- line 28: There's a mismatch between the test name and the description in `act`: We're checking wrong port number rather than missing configs.
- line 45: As above.
- line 63: As above.
- line 71: We should be checking `transport_security` rather than `auth_type`, right?
- line 115, 116: Do we need these lines? We don't need the charm to be in ActiveStatus to create relations. This comments actually applies to most tests.
- line 122: I think this could be a separate test.
- line 152: I think this could be a separate test.
- line 185: Is there any reason why we check the passwords only and we ignore other configs?
2. test_charm_state.py
- line 14: Please see my first question. Seems like a lot of maintenance in we want to add/remove some config options in the future. Isn't validating against the model enough?
3. test_library_smtp.py
- line 19, 54: Test for the provider side is missing.
- line 109: Typo in test name? Guess it should be `_if_no_data`.
- line 125: As above.
- line 141, 188, 226: Do we care whether the unit is leader or not?
- line 178: Please see question 7. in the `LIB` section.
GITHUB WORKFLOWS:
1. auto_update_libs.yaml - Do we need this workflow? This charm doesn't use any external libs.
I tried to keep the notes organized, but if anything seems unclear please feel free to ping me.
I’ve addressed several points and recreated the review PR. Would you mind posting the remaining issues there so that we can discuss them?
You can find below what has already been addressed struckthrough and some replies inline:
charm_state.py
Do new need KNOWN_CHARM_CONFIG in the charm_state.py? Can’t we just validate whatever config is given against the model?
Not sure about the name CharmState. To me, it looks more like a CharmConfigValidator
charm.py
Do we need to handle smtp and smtp_legacy separately?
Not sure about using saml in the context of the relations. I think it’s a bit confusing. I’d stick to SMTP.
renovate.json
Any reason why do we mention rockcraft.yaml? There’s a lot of unnecessary stuff there. Please review the file.
LIB:
In the main docstring, Provider Charm section, should the example also show how to use update_relation_data?
In the main docstring, Provider Charm section, provider charm doesn’t need to import SmtpDataAvailableEvent.
In the main docstring, imports are missing smtp after charms.smtp_integrator.v0. self.smtp declaration from line 22 also needs fixing.
Not sure about the relations property in SmtpProvides. IMO relations belong to the charm not the library. I’d move it to the charm.
IMO warnings about insecure auth_type and transport_security come far too late. How about placing them together with config validation?
In SmtpRequires, if relation data validation fails, we don’t get any feedback.
Do we need get_relation_data in SmtpRequires? Shouldn’t the smtp_data_available event be used as a public interface here?
TESTS:
In general, it feels like the docstrings could provide a bit more context about what’s exactly happening in the tests.
Nuances of smtp and smtp legacy can be really confusing; when saying invalid data, it would be good to explain why/how the data is invalid.
test_charm.py
line 28: There’s a mismatch between the test name and the description in act: We’re checking wrong port number rather than missing configs.
line 45: As above.
line 63: As above.
line 71: We should be checking transport_security rather than auth_type, right?
line 115, 116: Do we need these lines? We don’t need the charm to be in ActiveStatus to create relations. This comments actually applies to most tests.
line 122: I think this could be a separate test. Existence is already tested in another test, if that’s what you mean
line 152: I think this could be a separate test. Same as above
line 185: Is there any reason why we check the passwords only and we ignore other configs? The rest of the configs have already been tested in previous methods. The password vs password_id logic is dependent on the Juju version
test_charm_state.py
line 14: Please see my first question. Seems like a lot of maintenance in we want to add/remove some config options in the future. Isn’t validating against the model enough?
test_library_smtp.py
line 19, 54: Test for the provider side is missing.
line 109: Typo in test name? Guess it should be _if_no_data.
line 125: As above.
line 141, 188, 226: Do we care whether the unit is leader or not? These are actually regression tests
line 178: Please see question 7. in the LIB section.
GITHUB WORKFLOWS:
auto_update_libs.yaml - Do we need this workflow? This charm doesn’t use any external libs. This repository is bootstraped automatically and this file is part of the scaffolding
@gmerold Thanks for the detailed review! Once you are good with the changes Arturo has committed can you please post the checklist with approval(an example in the comments here)? Then we can go ahead with the listing
Hi @mcjaeger, we’re actively working on the review in this PR. I think we’re almost there. Once all the comments in the PR are resolved, I’ll post the checklist.
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).
Hello @arturo-seijas just wondered in this case, would you be willing to give also a short demo/explanation of the charm to the charming community - There are sessions possible on the Thursday community slot.
Apart from that, could we ask you @odysseus-k to switch this charm to listed please?
Will move the questions there. Thanks.