search

bradleyfalzon / gopherci

GopherCI was a project to help you maintain high-quality Go projects, by checking each GitHub Pull Request, for backward incompatible changes, and a suite of other third party static analysis tools.
https://gopherci.io
BSD 2-Clause "Simplified" License
102 stars 13 forks source link

Analysis page should better handle analysis failures #88

Closed bradleyfalzon closed 7 years ago

bradleyfalzon commented 7 years ago

Currently, if an analysis fails, the analysis page still tries to display diffs using the GitHub API if they fail, then the page returns a 500.

Instead, more thought needs to go into this error handling, such as recording the error string in the database so that can be displayed, and if the status is a failure, displaying that error. Also, if the diff API calls fail, still show some of the information.

Example: https://gci.gopherci.io/analysis/114

Log:

error getting diff from VCS for analysisID 114: GET https://api.github.com/repositories/16196540/compare/...: 404 Not Found []

Relates to #85.

dmitshur commented 7 years ago

Was that the entire error message or did you redact some parts? I'm curious what exact commits it was trying to compare and why it was 404.

bradleyfalzon commented 7 years ago

Sorry, I didn't place the full logs there as the issue was more about poor handling of errors, not the error itself.

bradleyfalzon commented 7 years ago

OK, this ticket is pretty broad, so I'm just going to handle the following:

Most of these are hard (5xx) errors because GopherCI fails to fetch the diff. It's failing to fetch the diff because every failed analysis never sets the relevant information for the commits (from/to or pull request number).

Fix: Set the commitFrom/commitTo/requestNumber when an analysis is created (we already have the information at that point in time).

Another scenario is when the commits have been forced pushed, and GopherCI can't fetch the diff because GitHub can't provide it. This isn't a critical error for GopherCI, we still have the results from the tools, and can just show them without also showing a diff, and it's essentially marked as a TODO https://github.com/bradleyfalzon/gopherci/blob/03badd2364a4a3e85c0e4389c88ca86b7e2f7a70/internal/web/web.go#L90-L92

Fix: If an error occurs getting the diff or making patches, just return the list of issues to the template.

There are other errors that can occur during execution, a common one is when a clone fails (see #85), but regardless, these errors should be presented to the user for them to decide whether it was a legitimate failure (yes the project just doesn't build) or a GopherCI bug (or something GopherCI could handle better). Currently, there's no feedback to the user.

Fix: All commands executed by GopherCI should be logged with their arguments, exit code, output and execution time. The web interface should then show this when viewing a build. There's a lot of output here sometimes, so we may just keep the output for a short period of time. I'm thinking about doing this via some kind of teeExecuter provided by the *SQLDB which can wrap another executor.