frontend: Warn when multiple rate limiters configured
Created by: ryanslade
We'll display a warning if more than one rate limiter is configured for a single code host:

NOTE: The issues below have now been solved by using the ValidateConfig
in frontend/db
There are a couple of issues which are not blocking, but would be nice to solve later:
One, when saving external service config the warning is not displayed immediately, only when you navigate away from the page or refresh.
Two, we need to load rate limit config in both repo-updater, to actually enforce the limits and now the frontend to display warnings. I moved common code to the extsvc
package to help with this. However, we can't do the DB call in extsvc
since we have two db packages, one from frontend and one from repo-updater. Importing either in extsvc
leads to an import cycle. Also, both frontend
and repo-updater
have their own definitions for the ExternalService
struct so we need to pull out the common fields and pass those to the extsvc
package.
(As a side note, we actually have a third definition of ExternalService
here: https://github.com/sourcegraph/sourcegraph/blob/8bea5eb42c72423a163932d6f31d9b5cf9ebb21d/internal/api/api.go#L118)
Ideally we should consolidate the different definitions of external service to a central package but I don't think we should do that as part of this PR.
Closes: https://github.com/sourcegraph/sourcegraph/issues/10026
Merge request reports
Activity
Created by: ryanslade
One, when saving external service config the warning is not displayed immediately, only when you navigate away from the page or refresh.
To address that, would it be simple to add it also to the
warning
property in the GQL resolver ofupdateExternalService
?That was actually what I tried first, see: https://gitlab.sgdev.org/root/sourcegraph/-/issues/10026#issuecomment-627444625
Created by: eseliger
I see, one answer to (1) would be to use markdown in the warning, it's already rendered as markdown in the webapp. Ex:
# Error one This is really serious! # Error two Another veery bad problem
type warning struct { warning string } func (w *warning) AddWarning(title, body string) { w.warning += fmt.Sprintf("# %s\n%s\n", title, body) }
I agree with that it should also be a banner, as this is a bad config error. Would be nice to immediately include it in there as well, though. Especially because it's immediately visible to the user :)
But that won't block this PR, we can file a separate ticket for it to keep track of it.
Deferring the actual review to core people
Created by: mrnugget
What's the reasoning against using this?
It's used when creating
and when updating an external service in the database:
The errors are displayed in the warning automatically. No string formatting etc. needed, since the multierror output is displayed correctly.
Created by: ryanslade
What's the reasoning against using this?
I didn't know about it
When I asked about how to tackle this I was pointed towards the approach I've taken in this PR: https://gitlab.sgdev.org/root/sourcegraph/-/issues/10026#issuecomment-627448627
I'll take a look into your suggested approach now