Skip to content

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Sep 23, 2025

This PR resolves #11893 by introducing a BANNER_MESSAGE environment variable. If set, the content will be available at https://crates.io/api/v1/site_metadata, which is already checked by our frontend to show a banner if the service is in read-only mode. The third commit adjusts the frontend logic to display the banner_message content instead, if it is set.

/cc @carols10cents

@Turbo87 Turbo87 added the C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear label Sep 23, 2025
@Turbo87 Turbo87 requested a review from a team September 23, 2025 15:34
Copy link
Contributor

@eth3lbert eth3lbert left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the content yet, but from the description, it seems this proposal leverages an environment variable to change the banner message. I'm not sure how much I like this, as it implies that every time a banner changes, we'd need to restart the application. I wonder if it would be more appealing to use a table accompanied by either a queue or pg_notify for this job, where an admin could just make a request to change the banner. However, this might be overkill if we only need to change the banner very rarely.

@Turbo87
Copy link
Member Author

Turbo87 commented Sep 24, 2025

However, this might be overkill if we only need to change the banner very rarely.

yeah, I assume we would rarely use this. the only advantage would be that regular admins could set the message without needing infra admin permission, but I'd say we can probably retrofit this if we really need this. since we have an on-call rotation with pagers the security WG will be able to quickly notify us to set the message for them, if necessary :)

Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

LGTM, this gives us the functionality we didn't have before!

I think @eth3lbert 's points are good though, for the long term-- ideally in the admin web interface of dreams, we'd have an easy way to set this on mobile while at cirque du soleil without needing to log in to heroku. But this is good enough for now!

@Turbo87 Turbo87 enabled auto-merge September 25, 2025 16:22
@Turbo87 Turbo87 merged commit b42f48c into rust-lang:main Sep 25, 2025
11 checks passed
@Turbo87 Turbo87 deleted the banner branch September 25, 2025 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ A-frontend 🐹 C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have a way to quickly put up a banner on every page
4 participants