Skip to content

Logs enhancement #488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 28, 2019
Merged

Logs enhancement #488

merged 1 commit into from
Jan 28, 2019

Conversation

dpordomingo
Copy link
Contributor

@dpordomingo dpordomingo commented Jan 24, 2019 •
edited
Loading

fix #487

before:

ERROR event processing failed
app=lookout
event-id=b44c158d09bebb40b85ce23763dd9ab179926019
event-type=*pb.ReviewEvent
github.pr=63
head=refs/pull/63/head
repo=https://github.com/dpordomingo/testing-repo.git
error=posting analysis failed: %!s(PANIC=runtime error: invalid memory address or nil pointer

problems:

  • no info about what caused the error
  • error message is a PANIC without trace

after

ERROR event processing failed

.... (same data as before changes)

error=posting analysis failed: github api error: review could not be pushed:
    POST https://api.github.com/repos/dpordomingo/testing-repo/pulls/63/reviews
    422 Unprocessable Entity
    [{Resource:Review
      Message:Path is invalid
    }]

benefits:

  • error message contains where was caused the problem
    • GitHub API endpoint
    • GitHub API real error message

these ☝️ new details would have been really useful when debugging #486

cons:

  • the workaround done inside handleAPIError could be done directly in google/go-github project, but it should be considered separately, so I'd handle it in a google/go-github PR

@dpordomingo dpordomingo added the enhancement New feature or request label Jan 24, 2019
@dpordomingo dpordomingo self-assigned this Jan 24, 2019
carlosms
carlosms reviewed Jan 24, 2019
View reviewed changes
carlosms
carlosms reviewed Jan 24, 2019
View reviewed changes
carlosms
carlosms reviewed Jan 24, 2019
View reviewed changes
@dpordomingo dpordomingo force-pushed the logs-enhancement branch 4 times, most recently from 55cb74a to eacee66 Compare January 25, 2019 16:10
@dpordomingo dpordomingo mentioned this pull request Jan 25, 2019
Open
@dpordomingo
Copy link
Contributor Author

As I said #488 (comment) I moved the disputed commit to a new PR #492 so the enhancements proposed by this one wont be blocked 🗡️

@dpordomingo dpordomingo requested a review from carlosms January 25, 2019 16:36
lookout-staging[bot]
lookout-staging bot reviewed Jan 28, 2019
View reviewed changes
carlosms
carlosms reviewed Jan 28, 2019
View reviewed changes
@dpordomingo dpordomingo requested a review from carlosms January 28, 2019 13:16
carlosms
carlosms approved these changes Jan 28, 2019
View reviewed changes
Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 with the change in the error message

@dpordomingo
Add more detail about 'ErrGitHubAPI'
accc8f4 
Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
@dpordomingo
Copy link
Contributor Author

I have no clue about why integration tests are now failing...

@dpordomingo
Copy link
Contributor Author

I saw it's already failing in master since last Friday
https://travis-ci.org/src-d/lookout/builds/484454336

@dpordomingo dpordomingo merged commit 438c784 into src-d:master Jan 28, 2019
@dpordomingo dpordomingo deleted the logs-enhancement branch January 28, 2019 19:46
@dpordomingo
Copy link
Contributor Author

And it is now passing.
https://travis-ci.org/src-d/lookout/builds/485567163
Heisenbug?
image

@carlosms
Copy link
Contributor

It's probably because of #490.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@se7entyse7en se7entyse7en Awaiting requested review from se7entyse7en

2 more reviewers

@lookout-staging lookout-staging[bot] lookout-staging[bot] left review comments

@carlosms carlosms carlosms approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@dpordomingo dpordomingo

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cryptic errors in some situations
2 participants
@dpordomingo @carlosms