Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add smtp relation interface #126

Merged
merged 34 commits into from
Dec 15, 2023
Merged

Add smtp relation interface #126

merged 34 commits into from
Dec 15, 2023

Conversation

arturo-seijas
Copy link
Contributor

@arturo-seijas arturo-seijas commented Dec 4, 2023

Add SMTP interface definition, as implemented by the SMTP integrator charm. A sample charm consuming this interface can be found here.
This relation interface defines how the SMTP details are shared with the required charm. Note that it supports both Juju Secrets and no secrets for legacy charms.

The spec defining this interface can be found here

@arturo-seijas
Copy link
Contributor Author

@PietroPasotti @simskij @gruyaume , can you please review?

Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing a good linter won't fix for you, but we're not there yet

README.md Show resolved Hide resolved
interfaces/smtp/v0/README.md Outdated Show resolved Hide resolved
interfaces/smtp/v0/README.md Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Show resolved Hide resolved
interfaces/smtp/v0/schema.py Show resolved Hide resolved
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more thingy I missed, my bad

interfaces/smtp/v0/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gruyaume
Copy link
Contributor

gruyaume commented Dec 5, 2023

@PietroPasotti @simskij @gruyaume , can you please review?

Please add a description to the PR. I.e. What am I reviewing here and why?

interfaces/smtp/v0/README.md Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
Copy link
Contributor

@gruyaume gruyaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as mentioned before)
Please add a description to the PR.

=4 Outdated Show resolved Hide resolved
@benhoyt
Copy link
Collaborator

benhoyt commented Dec 5, 2023

@arturo-seijas I'm going to be a grumpy old man for a second and double down on what @gruyaume said: you're requesting our review time (maybe 20-30 minutes each), so if you could please put in a decent amount of effort on the PR description to set context, that would be much appreciated. Simply saying "Add SMTP interface definition" doesn't add anything significant over the PR title. It may only need to be two or three sentences. Something like:

IS DevOps has developed the xyz charm that provides an SMTP server. This relation interface is useful for charms connecting to such servers, providing host/port/auth details for the requiring charm. For example, see our recent charms abc and def for example consumers. Note that the relation interface being proposed supports both Juju Secrets (preferred) and no Secrets (as we still have some legacy charms) -- see README.md for more details.

All this might "go without saying" from your point of view, but for reviewers being pulled in, context and links like that are very helpful.

Also worth considering when tagging people for review is to say why you're asking them. It's somewhat obvious why you would want to tag Charm Tech (@canonical/charm-tech), but perhaps "@PietroPasotti could you please review the Scenario tests specifically? And @gruyaume we'd love your take as I know the Telco team has used SMTP in the past." (or whatever reason)

docs/json_schemas/smtp/v0/provider.json Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Show resolved Hide resolved
interfaces/smtp/v0/schema.py Show resolved Hide resolved
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments that tbh are more from someone long in the email world than specifically charm-tech comments :)

interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/README.md Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/README.md Outdated Show resolved Hide resolved
interfaces/smtp/v0/README.md Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
interfaces/smtp/v0/README.md Outdated Show resolved Hide resolved
=4 Outdated Show resolved Hide resolved
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for making those adjustments. One tiny nit and one I assume accidental file add.

interfaces/smtp/v0/schema.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now (that definitely get @simskij's final review). And thanks for the update to the PR description.

@gruyaume gruyaume merged commit dff71d3 into canonical:main Dec 15, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants