Implement publish notification emails #9341
Conversation
Most `created_at` rows in the database default to `now()`, which can cause `insta` snapshots to be unstable when the content changes between runs due to the nature of time itself. This change adjusts the `emails_snapshot()` fn to replace the ISO8601 dates with a placeholder, which should cause the snapshots to become stable.
Turbo87
added
C-enhancement ✨
Turbo87
force-pushed
the
publish-notifications
branch
from
baf687d to
4c00c3c
Compare
| // Find names and email addresses of all crate owners | ||
| let recipients = crate_owners::table | ||
| .filter(crate_owners::deleted.eq(false)) | ||
| .filter(crate_owners::owner_kind.eq(OwnerKind::User)) |
There was a problem hiding this comment.
Should we also be looking up team owners?
There was a problem hiding this comment.
we should, I'm just not sure that we can... :D
our publishing code currently performs a "get all owner teams, iterate through teams and query GitHub API to see if publisher is member of the team" operation, but that does not list the members of these teams. in other words: we'd need new GitHub API calls since we don't "cache" the team memberships on our side. if/when we have non-GitHub teams implemented this will probably become a lot easier.
There was a problem hiding this comment.
Ugh, I forgot that we don't cache that. Sorry, some Sourcegraph slipped through there.
Not blocking for this PR, but I'm wondering if it's worth the effort to figure that1 out sooner rather than later — given that part of the motivation here (at least from what I see) is supply chain security, not sending notifications for crates that are entirely owned by teams (such as the AWS crate ecosystem) feels problematic.
I'll give this some thought.
Footnotes
-
For clarity, I'm more thinking just some level of caching here; not so much a full blown decoupled-from-GitHub team design. ↩
There was a problem hiding this comment.
I'm wondering if it's worth the effort to figure that out sooner rather than later
it then becomes a problem of cache invalidation. I think I'd personally rather see a proper teams concept in crates.io with a feature to optionally keep the team memberships in sync with some GitHub team. Pietro mentioned a while ago that this sync might be possible by having people install a GitHub app (?) in their orgs, but I haven't looked at any of this in detail yet.
sure, but when you look at other package registries, they also don't seem to allow you to turn this off. if you really wanted to ignore these notifications it should be easy to implement a corresponding filter in your email server/client. |
Oh, I don't actually think we should provide the option. I just expect someone to ask. 😅 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9341 +/- ##
==========================================
+ Coverage 89.19% 89.21% +0.01%
==========================================
Files 282 283 +1
Lines 28964 29071 +107
==========================================
+ Hits 25834 25935 +101
- Misses 3130 3136 +6 ☔ View full report in Codecov by Sentry. |
These were caused by a conflict between rust-lang#9342 and rust-lang#9341. Should've rebased first... 🙈
I started out rebasing #2705, then updated it to the current code structure, and moved all of the email sending to a background job.
In short: when a new crate version is published, a corresponding
send_publish_notificationsbackground job containing theversion_idwill be queued. that background job will query the relevant publish information (crate name, version, publish time, and publisher name) from the database, will get the email addresses of all current owners of the crate, and then send publish notifications to each of these owners. If sending all notification fails we assume a network issue and retry the job. If only a subset fails we assume something else is wrong and won't retry.