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
Will move the questions there. Thanks.