Review smtp-integrator charm for Listing

Metadata links

  • project repository:

https://github.com/canonical/smtp-integrator-operator

CI Links

  • Code linting or-style checks:

https://github.com/canonical/smtp-integrator-operator/blob/main/.github/workflows/test.yaml

  • Release automation:

https://github.com/canonical/smtp-integrator-operator/blob/main/.github/workflows/publish_charm.yaml

  • Unit test implementation:

https://github.com/canonical/smtp-integrator-operator/blob/main/.github/workflows/test.yaml

  • Unit test results:

https://github.com/canonical/smtp-integrator-operator/actions/runs/6651837446

  • Integration test implementation:

https://github.com/canonical/smtp-integrator-operator/blob/main/.github/workflows/integration_test.yaml

  • Integration test results:

https://github.com/canonical/smtp-integrator-operator/actions/runs/6651837445

Documentation Links

  • Usage:
  • Contribution:
  • Licensing:

https://github.com/canonical/smtp-integrator-operator/blob/main/LICENSE

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)

Hello,

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.

It’s a very nice charm overall. Good job!

BR,
Bartek

Hi @gmerold !

Thanks for the review.

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

  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 :wink:

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. 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
  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? These are actually regression tests
    • 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. This repository is bootstraped automatically and this file is part of the scaffolding

Hi @arturo-seijas,

Perfect, GH PRs are so much easier :smiley: Will move the questions there. Thanks.

BR,
Bartek

@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 :slight_smile:

Will do @varshigupta