Expand OIDC email template's publisher specifiers by Martolivna · Pull Request #13667 · pypi/warehouse (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation26 Commits7 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This adds details about which repository is trusted and the environment name to "trusted publisher added/removed" emails.

Resolves #13577

This adds details about which repository is trusted and the environment name to "trusted publisher added/removed" emails.

Resolves pypi#13577

Thanks @Martolivna! One small nitpick about how we'll end up rendering environments, but looks good overall!

This change prevents the optional 'environment' specifier from being rendered when it is empty.

Resolves pypi#13577

Thank you, @woodruffw! I've made some updates for conditionally rendering.

@woodruffw, I've made some updates. Your correction is really nice! Is there anything else I can do to improve it?

I've made some updates. Your correction is really nice! Is there anything else I can do to improve it?

Nope, looks great to me!

I've approved but I don't have the commit bit, so one of the maintainers will need to do the merge 🙂. But great work!

I've made some updates. Your correction is really nice! Is there anything else I can do to improve it?

Nope, looks great to me!

I've approved but I don't have the commit bit, so one of the maintainers will need to do the merge slightly_smiling_face. But great work!

That's wonderful! It was a pleasure working with you. Thank you for your assistance!

You as well! Thanks for tackling that issue 🙂

Just needs the result of make translations committed and this is good to go.

@woodruffw do we expect other (future) publishers to have all the same fields? Any idea what that'd look like? Will that need rethinking how this information is rendered?

This adds new lines of updated templates to the locale file.

Resolves pypi#13577

@ewdurbin could you smash that “Run CI” button, please?

do we expect other (future) publishers to have all the same fields? Any idea what that'd look like? Will that need rethinking how this information is rendered?

Nope, they'll all be slightly different, unfortunately. I think it's okay to hard-code this for now, since we have other in-flight work (e.g. #13569) that'll need to update these emails to make them generic over different providers.

(Eventually, the rendering will probably need to become fully independent of each provider type -- I need to give some more thought to how that should look.)

Eventually, the rendering will probably need to become fully independent of each provider type -- I need to give some more thought to how that should look.

Yeah, makes sense. I've been talking to Marta privately, throwing some ideas around, and thought that there could be a @property on the models that would return a string to be rendered. Though, she also suggested such a property to return a dict so that keys+values could still be rendered as a table in a loop.

there could be a @property on the models that would return a string to be rendered. Though, she also suggested such a property to return a dict so that keys+values could still be rendered as a table in a loop.

Yeah, this is close to what I've been thinking too -- we could probably do this generically by inspecting Model.__table__ or using the SQLAlchemy introspection APIs, but both of those might be too magical. We'll see as we get closer to landing other trusted publisher providers 🙂