campaigns: suggest alternatives when campaigns are unlicensed #410
Conversation
There was a problem hiding this comment.
Code looks good to me and I do appreciate the work you put into this, which seems like a lot, but I'm also skeptical of whether we should do this.
It adds a lot of complexity (errors being wrapped, errors being printed differently, multiple ways to get suggestions, additional requests being made) that I'm not sure is worth it, because in the end the suggestions help to make the error go away, but they might not be what the user is after. And more often than not, adding a count:x fixes the problem. Or using a repo: filter, or a lang: filter, all of which is already suggested in our web UI to drill down on search results.
LawnGnome
force-pushed
the
aharvey/improve-licensing-error-output
branch
from
26d15b7 to
4d2e549
Compare
LawnGnome
dismissed
mrnugget’s stale review
No longer relevant with the changes today.
|
This has been refactored into the simpler version that just gives you a couple of static hints, as discussed at today's sync, and I've dismissed the existing review, since the PR has changed a fair bit. |
| if ext != nil { | ||
| if ext["code"] == nil { | ||
| return "", nil | ||
| } else if code, ok := ext["code"].(string); ok { | ||
| return code, nil | ||
| } | ||
| return "", errors.Errorf("unexpected code of type %T", ext["code"]) | ||
| } |
There was a problem hiding this comment.
whoa so complicated 😬
Great though :)
In and around sourcegraph/sourcegraph#16687, we had a fairly lengthy discussion around how to present licensing errors to the user. Doing it in a way that is actionable for the user is fairly tricky: we don't have a simple limit directive that could be applied to the repository search, so the user is left with a campaign spec that they might still want to trial but don't know how to get to a trialable state.
This PR uses the new error code returned from sourcegraph/sourcegraph#16687 to special case licensing errors. When those are detected, a suggestion is made for how the campaign spec could be modified to be compliant with the unlicensed behaviour.
Here's how it looks:
Linked to sourcegraph/sourcegraph#15715, but it's more a bonus adventure than that issue.