Merged
Conversation
2db4cc2 to
cf01c9f
Compare
GregorShear
commented
Mar 26, 2026
ab1265c to
f4a0094
Compare
f4a0094 to
5cf384d
Compare
5cf384d to
4ead7e9
Compare
jshearer
requested changes
Apr 2, 2026
Contributor
There was a problem hiding this comment.
- What are we doing about
ShardFailed? Currently, we have been treating it as a beta feature that customers can opt into by asking us. This PR marks it as both default and system, meaning that not only is it on by default, you actually can't turn it off. That seems surprising. Did you talk about this w/ Dave? - There are now two different sources of truth for the default alerts: this
AlertType::is_default()and the database column defaults. And in fact sinceShardFailedis marked as default in the PR, those are divergent now. Today, the DB column default is what gets used when provisioning new tenants. I note that despite introducing a concept calledis_default, this PR doesn't actually change the behavior of the system to use it anywhere. - How are we thinking about the migration here now that we're introducing
is_system/is_default? Once we merge this, there will be a lot of existing tenants with, for example, missing alert subscriptions that are marked asis_system. Will they be able to... add them in the UI, but just not remove them? Or what happens if someone modifies this in the DB, which does happen. - There's an implicit relationship between system and default alerts: every system alert is also necessarily a default alert, but not every default alert is necessarily a system alert. It might be worth modeling this, or at least commenting on it? WDYT?
- I don't love how the description string can be/is different from the doc-comment for these alerts. I wonder if there's any way you/Claude can find to "reflect" the doc comment here? If not no big deal, but IMO it would be worth at least trying.
Put another way, I don't think this is coherent with the existing architecture, and I'm asking you to re-think how it should work so we don't introduce competing concepts and tech debt.
Contributor
Author
|
3edd117 to
3c0bacd
Compare
Remove alert type doc strings in favor of user-facing descriptions Reset shardFailed to default=false
3c0bacd to
b1367ef
Compare
jshearer
approved these changes
Apr 2, 2026
Contributor
jshearer
left a comment
There was a problem hiding this comment.
Okay, this is looking better. The last thing I was concerned about is that removing the default on public.alert_subscriptions will actually break certain POST /rest/v1/alert_subscriptions calls that rely on the default value for include_alert_types. But you say you checked and the UI is purely using gql to create alert subscriptions, so with that I think this is good to go 👍
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
alertTypesGraphQL query that returns all possible alert types with user-facing metadata (title, description)NOTE: Pagination will not be implemented for this endpoint because of the simplicity of the data returned.
Issues
The issues directly below are completely resolved by this PR:
#2794