Skip to content

Conversation

Martolivna
Copy link
Contributor

@Martolivna Martolivna commented May 15, 2023

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
@Martolivna Martolivna requested a review from a team as a code owner May 15, 2023 11:52
@woodruffw
Copy link
Member

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
@Martolivna
Copy link
Contributor Author

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

@Martolivna
Copy link
Contributor Author

Martolivna commented May 15, 2023

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

@woodruffw
Copy link
Member

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!

@Martolivna
Copy link
Contributor Author

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!

@woodruffw
Copy link
Member

You as well! Thanks for tackling that issue 🙂

@ewdurbin
Copy link
Member

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

@webknjaz
Copy link
Member

@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
@webknjaz webknjaz added UX/UI design, user experience, user interface usability trusted-publishing labels May 15, 2023
@webknjaz
Copy link
Member

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

@woodruffw
Copy link
Member

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.)

@webknjaz
Copy link
Member

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.

@webknjaz webknjaz requested a review from ewdurbin May 15, 2023 20:56
@woodruffw
Copy link
Member

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 🙂

@ewdurbin ewdurbin enabled auto-merge (squash) May 16, 2023 18:34
@ewdurbin ewdurbin merged commit dcc776a into pypi:main May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
trusted-publishing usability UX/UI design, user experience, user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trusted publishing: "OIDC added" email does not specify full trust details
4 participants