Skip to content

status-indicator: Display external service syncing errors

Warren Gifford requested to merge core/ext-svc-errors-2 into master

Created by: mrnugget

This is the revised version of the (never merged) PR #4780 (closed) and the first attempt at fixing #5146 (closed).

TL;DR: This is step 1/n. It shows repo-updater errors in the UI. They look raw, but I think that's fine, because we finally show errors and I'm now also happy with the underlying implementation and think that we can iterate on the details (see later sections)

What this PR does it to make the errors visible that repo-updater encounters when syncing repositories from external services. These errors can be produced when trying to construct an external service from the existing configuration or when talking to the external service to list the available repositories for sync.

These errors then get shown in the UI, like this:

Screen Shot 2019-08-21 at 17 05 29

Implementation

The implementation in this PR is based on #4780 (closed):

  • It adds a new type of status message, a SyncErrorStatusMessage
  • This status message is returned by repo-updater in its /status-messages endpoint when a sync error has been produced in the last sync run
  • Each sync run sets or resets the current sync error, meaning: fixing an external service configuration results in the message disappearing with the next successful sync (triggered on save). But that also means that errors that pop up long after an external service has been created are displayed (network errors, invalid tokens, ...)

In addition to that it also contains the suggestions made by @felixfbecker and @tsenart:

  • The StatusMessage type in the GraphQL schema has been turned into a union type. I like this approach a lot more, because it not only gives us type-safety in TypeScript, but (at the cost of verbosity) also gives us better type-safety in graphqlbackend and repo-updater.
  • There are no union types in Go, which makes the definition of StatusMessage in repoupdater/protocol a bit unwiedly. But that's one of the possible I discussed with @tsenart and I like it a lot more than the untyped list of key-value pairs.
  • The sync errors are now styled with alert-danger which makes them pop out a lot more as errors

Open issues, drawbacks & thoughts for enhancements

  1. The error messages are still raw. But now that I'm happier with the underlying implementation I agree with @nicksnyder's assessment that showing anything in case there's an error is better than showing nothing. And It's actually not that easy to untangle layers and layers of errors produced by (possibly multiple) external services and turn them into something meaningful and actionable. But I will use the rest of this iteration to hack on this problem. Idea: I prototyped to define a set of errors that is common across external services (bad credentials, connection error, rate limit, ...) and then wrap the errors produced by each external service into one of these custom errors. In the repo-updater endpoint we could then detect these errors and replace their messages with something meaningful. The drawback of that is that it's hard to define this common set so it makes sense for all external services and wrapping each the errors produced by each one requires knowledge of each external service's idosyncracies. That's a lot of work, but we could approach it step by step and wrap/beautify these errors as they arise.
  2. This does not (yet?) show the warnings that repo-updater produces. Example of such a warning is "repository not found" which is produced when we try to fetch the configured repositories but can't find one of them. This warning is only written to the log and never returned as an error, because we want to
  3. This does not (yet?) show errors/warnings that gitserver runs into when cloning/fetching/cleaning repositories. Until now I haven't looked into making gitserver errors visible, but that's certainly a large part of what would make fixing #5146 (closed) valuable

Especially 2 and 3 make me question whether the current "architecture" will hold up: if we only care about errors, it's easy to remember the last error and reset it when everything's fine again. But what do we do when we want to display ephemeral data, such as warnings? Where and how do we collect it? (Theoretically we could change our function's return signature from <val>, error to <val>, warningstring, error, but that's ... ugly). I keep thinking that we need to introduce something (an ErrorReporter?) that other functions/methods could call to report an issue. That would be far easier than wrapping, passing and unwrapping errors along the call chain in the syncer, with the possible added cost of introducing a singleton. And, still: what do we do with warnings?

Happy about any input/feedback/ideas!

Test Plan

Test plan: manual testing, go test, cd web && yarn test --runInBand StatusMessagesNavItem.test.tsx

Merge request reports

Loading