This repository has been archived by the owner on Sep 30, 2024. It is now read-only.
monitoring: disable critical alerts banner by default #12155
Merged
Conversation
This file contains 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
bobheadxi
requested review from
keegancsmith,
slimsag,
juliasourceress and
a team
Codecov Report
@@ Coverage Diff @@
## master #12155 +/- ##
=======================================
Coverage 50.66% 50.67%
=======================================
Files 1420 1420
Lines 80360 80360
Branches 6734 6680 -54
=======================================
+ Hits 40718 40721 +3
+ Misses 36061 36059 -2
+ Partials 3581 3580 -1
|
Codecov Report
@@ Coverage Diff @@
## master #12155 +/- ##
=======================================
Coverage 50.66% 50.67%
=======================================
Files 1420 1420
Lines 80360 80360
Branches 6734 6734
=======================================
+ Hits 40718 40721 +3
+ Misses 36061 36059 -2
+ Partials 3581 3580 -1
|
Co-authored-by: ᴜɴᴋɴᴡᴏɴ <[email protected]>
slimsag
reviewed
Jul 14, 2020
| @@ -175,7 +175,7 @@ | |||
| "alerts.hideObservabilitySiteAlerts": { | |||
| "description": "Disables observability-related site alert banners.", | |||
| "type": "boolean", | |||
| "default": false | |||
| "default": true | |||
There was a problem hiding this comment.
Sorry for the confusion, but these default values are not respected they only act as documentation. This had no impact on the actual default behavior - you need to handle that at the location where you use this setting in code
There was a problem hiding this comment.
We're still seeing this on sourcegraph.com because of this
There was a problem hiding this comment.
When you fix this, make sure you cherry-pick the commit into the 3.18 branch so it gets into the release as well
There was a problem hiding this comment.
Ah that's unfortunate - thanks for catching that! https://github.com/sourcegraph/sourcegraph/pull/12189
bobheadxi
referenced
this pull request
Jul 15, 2020
fixes https://github.com/sourcegraph/sourcegraph/pull/12155#discussion_r454655541 - setting the default in the schema is not sufficient
daxmc99
referenced
this pull request
Jul 15, 2020
fixes https://github.com/sourcegraph/sourcegraph/pull/12155#discussion_r454655541 - setting the default in the schema is not sufficient
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This banner is currently a significant source of confusion, and since we're still working on dogfooding alerts in sourcegraph.com (https://github.com/sourcegraph/sourcegraph/issues/5370) it's hard to say we're confident about all our alerts enough to have it displayed so prominently (yet)
This changes the default of
alerts.hideObservabilitySiteAlertstotrueMost recent Slack discussion: https://sourcegraph.slack.com/archives/C07KZF47K/p1594730258245600