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.

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

Comments on commits that can't be seen. #126

Closed
dmitshur opened this Issue Sep 18, 2017 · 10 comments

Comments

Projects
None yet
2 participants
@dmitshur

dmitshur commented Sep 18, 2017

This is either an issue with GopherCI or with GitHub, I can't immediately tell. Reporting here for investigation.

I'm seeing some notifications and signs of comments left on commits, yet when I visit that commit, I can't see any comments. Here's an instance that happened just now.

In my GitHub notifications:

image

At https://github.com/shurcooL/github_flavored_markdown/commits/master:

image

Note it shows there are 2 comments on that commit.

Yet the actual commit shows 0 comments:

image

@bradleyfalzon

This comment has been minimized.

Show comment
Hide comment
@bradleyfalzon

bradleyfalzon Sep 18, 2017

Owner

This symptom, comments on a commit that aren't visible, usually occurs when the commit comment API is used on a file or line that hasn't changed. So the commit does have two comments, but the UI only shows changed lines so the comments are missing.

So I'm going to suggest it looks like a bug on my end, placing comments on lines that didn't change. This has been tested extensively, though, so I do find this a little surprising.

It appears golint and apicompat raised the issues (based on logs), apicompat may have complained about an exported function being removed (although this is a main package, so it shouldn't have). I can't see immediately why golint would have had an issue with this change.

I'll need to look into this a little later, unfortunately the logging isn't extensive for this area. For my records:

{"installationID":5912,"level":"info","logger":"gci","msg":"analysing","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:09+09:30"}
{"analysisID":1049,"installationID":5912,"level":"info","logger":"gci","msg":"created new analysis record","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:09+09:30"}
{"analysisID":1049,"installationID":5912,"level":"info","logger":"gci","msg":"Setting https://api.github.com/repos/shurcooL/github_flavored_markdown/statuses/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9 state: \"pending\", context: \"ci/gopherci/push\", description: \"In progress\"","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:09+09:30"}
{"area":"docker","containerID":"f1c55b9b8d6ccb0d5401c28543b2534d7134d2a0e16b9021e6486929630d7b20","level":"info","logger":"gci","msg":"created exec for cmd: \u0026{9ca119cab4faef64553c96870ec33b1479b3a51ba3e01d53efd7be5d32e4531c}%!(EXTRA []string=[bash -c ulimit -v 1572864 \u0026\u0026 cd $GOPATH/src/github.com/shurcooL/github_flavored_markdown; git checkout cccd3ce4f8e394ae9f87de0bd8b37e00625913d9])","server_name":"www1","time":"2017-09-19T03:48:10+09:30"}
{"area":"docker","containerID":"f1c55b9b8d6ccb0d5401c28543b2534d7134d2a0e16b9021e6486929630d7b20","level":"info","logger":"gci","msg":"created exec for cmd: \u0026{cb88d577c385b96d6f41395a3e14923606ceb4fe34a0936a82e586b4b0c8754c}%!(EXTRA []string=[bash -c ulimit -v 1572864 \u0026\u0026 cd $GOPATH/src/github.com/shurcooL/github_flavored_markdown; git diff cccd3ce4f8e394ae9f87de0bd8b37e00625913d9~1...cccd3ce4f8e394ae9f87de0bd8b37e00625913d9])","server_name":"www1","time":"2017-09-19T03:48:11+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"No (or unsupported) vendoring tool, using go get","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"install-deps.sh","time":"2017-09-19T03:48:18+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"go vet","time":"2017-09-19T03:48:18+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:18+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"golint","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 1 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"main.go was not generated","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"isFileGenerated","time":"2017-09-19T03:48:19+09:30"}
{"area":"docker","containerID":"f1c55b9b8d6ccb0d5401c28543b2534d7134d2a0e16b9021e6486929630d7b20","level":"info","logger":"gci","msg":"created exec for cmd: \u0026{a72149de8bc6976a3b6b49dc26338260904a6684b52ef677d6c65b0581fc9b10}%!(EXTRA []string=[bash -c ulimit -v 1572864 \u0026\u0026 cd $GOPATH/src/github.com/shurcooL/github_flavored_markdown; apicompat -before cccd3ce4f8e394ae9f87de0bd8b37e00625913d9~1 ./...])","server_name":"www1","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"apicompat","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 1 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"main.go was not generated","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"isFileGenerated","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"gosimple","time":"2017-09-19T03:48:26+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:26+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"staticcheck","time":"2017-09-19T03:48:34+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:34+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"unused","time":"2017-09-19T03:48:40+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:40+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"unparam","time":"2017-09-19T03:48:46+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:46+09:30"}
{"analysisID":1049,"installationID":5912,"level":"info","logger":"gci","msg":"Setting https://api.github.com/repos/shurcooL/github_flavored_markdown/statuses/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9 state: \"success\", context: \"ci/gopherci/push\", description: \"Found 2 issues\"","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:46+09:30"}
Owner

bradleyfalzon commented Sep 18, 2017

This symptom, comments on a commit that aren't visible, usually occurs when the commit comment API is used on a file or line that hasn't changed. So the commit does have two comments, but the UI only shows changed lines so the comments are missing.

So I'm going to suggest it looks like a bug on my end, placing comments on lines that didn't change. This has been tested extensively, though, so I do find this a little surprising.

It appears golint and apicompat raised the issues (based on logs), apicompat may have complained about an exported function being removed (although this is a main package, so it shouldn't have). I can't see immediately why golint would have had an issue with this change.

I'll need to look into this a little later, unfortunately the logging isn't extensive for this area. For my records:

{"installationID":5912,"level":"info","logger":"gci","msg":"analysing","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:09+09:30"}
{"analysisID":1049,"installationID":5912,"level":"info","logger":"gci","msg":"created new analysis record","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:09+09:30"}
{"analysisID":1049,"installationID":5912,"level":"info","logger":"gci","msg":"Setting https://api.github.com/repos/shurcooL/github_flavored_markdown/statuses/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9 state: \"pending\", context: \"ci/gopherci/push\", description: \"In progress\"","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:09+09:30"}
{"area":"docker","containerID":"f1c55b9b8d6ccb0d5401c28543b2534d7134d2a0e16b9021e6486929630d7b20","level":"info","logger":"gci","msg":"created exec for cmd: \u0026{9ca119cab4faef64553c96870ec33b1479b3a51ba3e01d53efd7be5d32e4531c}%!(EXTRA []string=[bash -c ulimit -v 1572864 \u0026\u0026 cd $GOPATH/src/github.com/shurcooL/github_flavored_markdown; git checkout cccd3ce4f8e394ae9f87de0bd8b37e00625913d9])","server_name":"www1","time":"2017-09-19T03:48:10+09:30"}
{"area":"docker","containerID":"f1c55b9b8d6ccb0d5401c28543b2534d7134d2a0e16b9021e6486929630d7b20","level":"info","logger":"gci","msg":"created exec for cmd: \u0026{cb88d577c385b96d6f41395a3e14923606ceb4fe34a0936a82e586b4b0c8754c}%!(EXTRA []string=[bash -c ulimit -v 1572864 \u0026\u0026 cd $GOPATH/src/github.com/shurcooL/github_flavored_markdown; git diff cccd3ce4f8e394ae9f87de0bd8b37e00625913d9~1...cccd3ce4f8e394ae9f87de0bd8b37e00625913d9])","server_name":"www1","time":"2017-09-19T03:48:11+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"No (or unsupported) vendoring tool, using go get","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"install-deps.sh","time":"2017-09-19T03:48:18+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"go vet","time":"2017-09-19T03:48:18+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:18+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"golint","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 1 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"main.go was not generated","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"isFileGenerated","time":"2017-09-19T03:48:19+09:30"}
{"area":"docker","containerID":"f1c55b9b8d6ccb0d5401c28543b2534d7134d2a0e16b9021e6486929630d7b20","level":"info","logger":"gci","msg":"created exec for cmd: \u0026{a72149de8bc6976a3b6b49dc26338260904a6684b52ef677d6c65b0581fc9b10}%!(EXTRA []string=[bash -c ulimit -v 1572864 \u0026\u0026 cd $GOPATH/src/github.com/shurcooL/github_flavored_markdown; apicompat -before cccd3ce4f8e394ae9f87de0bd8b37e00625913d9~1 ./...])","server_name":"www1","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"apicompat","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 1 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"main.go was not generated","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"isFileGenerated","time":"2017-09-19T03:48:19+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"gosimple","time":"2017-09-19T03:48:26+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:26+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"staticcheck","time":"2017-09-19T03:48:34+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:34+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"unused","time":"2017-09-19T03:48:40+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:40+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"ran tool","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","step":"unparam","time":"2017-09-19T03:48:46+09:30"}
{"analysisID":1049,"area":"analyser","installationID":5912,"level":"info","logger":"gci","msg":"revgrep found 0 issues","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:46+09:30"}
{"analysisID":1049,"installationID":5912,"level":"info","logger":"gci","msg":"Setting https://api.github.com/repos/shurcooL/github_flavored_markdown/statuses/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9 state: \"success\", context: \"ci/gopherci/push\", description: \"Found 2 issues\"","owner":"shurcooL","pr":0,"ref":"cccd3ce4f8e394ae9f87de0bd8b37e00625913d9","repo":"github_flavored_markdown","server_name":"www1","time":"2017-09-19T03:48:46+09:30"}
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 18, 2017

apicompat may have complained about an exported function being removed (although this is a main package, so it shouldn't have)

It's not a main package (a command), it's package github_flavored_markdown (a library). The commit is indeed a breaking API change. It removes a (deprecated) exported function.

I can't see immediately why golint would have had an issue with this change.

I suspect it's:

github_flavored_markdown $ golint 
main.go:11:1: don't use an underscore in package name
sanitize_test.go:1:1: don't use an underscore in package name

It'd probably try to leave that comment on the package clause line.

(The underscore is unfortunate, but it's too expensive to change now. The package is very old and predates me being aware of best practices for Go package names.)

dmitshur commented Sep 18, 2017

apicompat may have complained about an exported function being removed (although this is a main package, so it shouldn't have)

It's not a main package (a command), it's package github_flavored_markdown (a library). The commit is indeed a breaking API change. It removes a (deprecated) exported function.

I can't see immediately why golint would have had an issue with this change.

I suspect it's:

github_flavored_markdown $ golint 
main.go:11:1: don't use an underscore in package name
sanitize_test.go:1:1: don't use an underscore in package name

It'd probably try to leave that comment on the package clause line.

(The underscore is unfortunate, but it's too expensive to change now. The package is very old and predates me being aware of best practices for Go package names.)

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 18, 2017

I see the 2 comments at via the API:

https://api.github.com/repos/shurcooL/github_flavored_markdown/commits/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9/comments

[
  {
    "url": "https://api.github.com/repos/shurcooL/github_flavored_markdown/comments/24381322",
    "html_url": "https://github.com/shurcooL/github_flavored_markdown/commit/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9#commitcomment-24381322",
    "id": 24381322,
    "user": {
      "login": "gopherci[bot]",
      "id": 24670144,
      "avatar_url": "https://avatars3.githubusercontent.com/in/889?v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/gopherci%5Bbot%5D",
      "html_url": "https://github.com/apps/gopherci",
      "followers_url": "https://api.github.com/users/gopherci%5Bbot%5D/followers",
      "following_url": "https://api.github.com/users/gopherci%5Bbot%5D/following{/other_user}",
      "gists_url": "https://api.github.com/users/gopherci%5Bbot%5D/gists{/gist_id}",
      "starred_url": "https://api.github.com/users/gopherci%5Bbot%5D/starred{/owner}{/repo}",
      "subscriptions_url": "https://api.github.com/users/gopherci%5Bbot%5D/subscriptions",
      "organizations_url": "https://api.github.com/users/gopherci%5Bbot%5D/orgs",
      "repos_url": "https://api.github.com/users/gopherci%5Bbot%5D/repos",
      "events_url": "https://api.github.com/users/gopherci%5Bbot%5D/events{/privacy}",
      "received_events_url": "https://api.github.com/users/gopherci%5Bbot%5D/received_events",
      "type": "Bot",
      "site_admin": false
    },
    "position": 0,
    "line": null,
    "path": "main.go",
    "commit_id": "cccd3ce4f8e394ae9f87de0bd8b37e00625913d9",
    "created_at": "2017-09-18T18:18:46Z",
    "updated_at": "2017-09-18T18:18:46Z",
    "author_association": "NONE",
    "body": "golint: don't use an underscore in package name"
  },
  {
    "url": "https://api.github.com/repos/shurcooL/github_flavored_markdown/comments/24381323",
    "html_url": "https://github.com/shurcooL/github_flavored_markdown/commit/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9#commitcomment-24381323",
    "id": 24381323,
    "user": {
      "login": "gopherci[bot]",
      "id": 24670144,
      "avatar_url": "https://avatars3.githubusercontent.com/in/889?v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/gopherci%5Bbot%5D",
      "html_url": "https://github.com/apps/gopherci",
      "followers_url": "https://api.github.com/users/gopherci%5Bbot%5D/followers",
      "following_url": "https://api.github.com/users/gopherci%5Bbot%5D/following{/other_user}",
      "gists_url": "https://api.github.com/users/gopherci%5Bbot%5D/gists{/gist_id}",
      "starred_url": "https://api.github.com/users/gopherci%5Bbot%5D/starred{/owner}{/repo}",
      "subscriptions_url": "https://api.github.com/users/gopherci%5Bbot%5D/subscriptions",
      "organizations_url": "https://api.github.com/users/gopherci%5Bbot%5D/orgs",
      "repos_url": "https://api.github.com/users/gopherci%5Bbot%5D/repos",
      "events_url": "https://api.github.com/users/gopherci%5Bbot%5D/events{/privacy}",
      "received_events_url": "https://api.github.com/users/gopherci%5Bbot%5D/received_events",
      "type": "Bot",
      "site_admin": false
    },
    "position": 0,
    "line": null,
    "path": "main.go",
    "commit_id": "cccd3ce4f8e394ae9f87de0bd8b37e00625913d9",
    "created_at": "2017-09-18T18:18:46Z",
    "updated_at": "2017-09-18T18:18:46Z",
    "author_association": "NONE",
    "body": "apicompat:  breaking change declaration removed"
  }
]

The comments are as suspected. One from golint, another from apicompat.

It seems they're missing position information:

"position": 0,
"line": null,
"path": "main.go",

It looks to me that there are 2 separate things happening here:

  1. GitHub should either display comments that have missing line information both on commits list page, and on the individual commit. Or not display them anywhere and don't send notifications. But not the inconsistent state now where you get a notification, see number of comments on commits list page, but can't actually see the comments. (Alternatively, creating such a comment should return an error.)

  2. You may or may not want to create such comments in GopherCI.

dmitshur commented Sep 18, 2017

I see the 2 comments at via the API:

https://api.github.com/repos/shurcooL/github_flavored_markdown/commits/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9/comments

[
  {
    "url": "https://api.github.com/repos/shurcooL/github_flavored_markdown/comments/24381322",
    "html_url": "https://github.com/shurcooL/github_flavored_markdown/commit/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9#commitcomment-24381322",
    "id": 24381322,
    "user": {
      "login": "gopherci[bot]",
      "id": 24670144,
      "avatar_url": "https://avatars3.githubusercontent.com/in/889?v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/gopherci%5Bbot%5D",
      "html_url": "https://github.com/apps/gopherci",
      "followers_url": "https://api.github.com/users/gopherci%5Bbot%5D/followers",
      "following_url": "https://api.github.com/users/gopherci%5Bbot%5D/following{/other_user}",
      "gists_url": "https://api.github.com/users/gopherci%5Bbot%5D/gists{/gist_id}",
      "starred_url": "https://api.github.com/users/gopherci%5Bbot%5D/starred{/owner}{/repo}",
      "subscriptions_url": "https://api.github.com/users/gopherci%5Bbot%5D/subscriptions",
      "organizations_url": "https://api.github.com/users/gopherci%5Bbot%5D/orgs",
      "repos_url": "https://api.github.com/users/gopherci%5Bbot%5D/repos",
      "events_url": "https://api.github.com/users/gopherci%5Bbot%5D/events{/privacy}",
      "received_events_url": "https://api.github.com/users/gopherci%5Bbot%5D/received_events",
      "type": "Bot",
      "site_admin": false
    },
    "position": 0,
    "line": null,
    "path": "main.go",
    "commit_id": "cccd3ce4f8e394ae9f87de0bd8b37e00625913d9",
    "created_at": "2017-09-18T18:18:46Z",
    "updated_at": "2017-09-18T18:18:46Z",
    "author_association": "NONE",
    "body": "golint: don't use an underscore in package name"
  },
  {
    "url": "https://api.github.com/repos/shurcooL/github_flavored_markdown/comments/24381323",
    "html_url": "https://github.com/shurcooL/github_flavored_markdown/commit/cccd3ce4f8e394ae9f87de0bd8b37e00625913d9#commitcomment-24381323",
    "id": 24381323,
    "user": {
      "login": "gopherci[bot]",
      "id": 24670144,
      "avatar_url": "https://avatars3.githubusercontent.com/in/889?v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/gopherci%5Bbot%5D",
      "html_url": "https://github.com/apps/gopherci",
      "followers_url": "https://api.github.com/users/gopherci%5Bbot%5D/followers",
      "following_url": "https://api.github.com/users/gopherci%5Bbot%5D/following{/other_user}",
      "gists_url": "https://api.github.com/users/gopherci%5Bbot%5D/gists{/gist_id}",
      "starred_url": "https://api.github.com/users/gopherci%5Bbot%5D/starred{/owner}{/repo}",
      "subscriptions_url": "https://api.github.com/users/gopherci%5Bbot%5D/subscriptions",
      "organizations_url": "https://api.github.com/users/gopherci%5Bbot%5D/orgs",
      "repos_url": "https://api.github.com/users/gopherci%5Bbot%5D/repos",
      "events_url": "https://api.github.com/users/gopherci%5Bbot%5D/events{/privacy}",
      "received_events_url": "https://api.github.com/users/gopherci%5Bbot%5D/received_events",
      "type": "Bot",
      "site_admin": false
    },
    "position": 0,
    "line": null,
    "path": "main.go",
    "commit_id": "cccd3ce4f8e394ae9f87de0bd8b37e00625913d9",
    "created_at": "2017-09-18T18:18:46Z",
    "updated_at": "2017-09-18T18:18:46Z",
    "author_association": "NONE",
    "body": "apicompat:  breaking change declaration removed"
  }
]

The comments are as suspected. One from golint, another from apicompat.

It seems they're missing position information:

"position": 0,
"line": null,
"path": "main.go",

It looks to me that there are 2 separate things happening here:

  1. GitHub should either display comments that have missing line information both on commits list page, and on the individual commit. Or not display them anywhere and don't send notifications. But not the inconsistent state now where you get a notification, see number of comments on commits list page, but can't actually see the comments. (Alternatively, creating such a comment should return an error.)

  2. You may or may not want to create such comments in GopherCI.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 18, 2017

I've reported the GitHub-facing issue to them via support@github.com.

dmitshur commented Sep 18, 2017

I've reported the GitHub-facing issue to them via support@github.com.

@bradleyfalzon

This comment has been minimized.

Show comment
Hide comment
@bradleyfalzon

bradleyfalzon Sep 19, 2017

Owner

OK, there are two issues here:

  1. revgrep should have excluded the issue main.go:11:1: don't use an underscore in package name, this was caused by revgrep incorrectly detecting the file as new, and I'll patch that shortly.
  2. Although apicompat correctly reported the issue, it shouldn't have, and that was due to #86 (and possibly #63). It reported the issue for the same reason above, revgrep incorrectly detected the file main.go as being a new file.

I'll fix revgrep now.

BTW, thanks for reporting this, it's greatly appreciated.

Owner

bradleyfalzon commented Sep 19, 2017

OK, there are two issues here:

  1. revgrep should have excluded the issue main.go:11:1: don't use an underscore in package name, this was caused by revgrep incorrectly detecting the file as new, and I'll patch that shortly.
  2. Although apicompat correctly reported the issue, it shouldn't have, and that was due to #86 (and possibly #63). It reported the issue for the same reason above, revgrep incorrectly detected the file main.go as being a new file.

I'll fix revgrep now.

BTW, thanks for reporting this, it's greatly appreciated.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 19, 2017

No problem. GitHub support (h/t @izuzak) got back to me, confirming the following:

  • Comments on a diff line with the index 0 are invalid. Diff lines start at 1. GopherCI should not be creating such invalid comments.

  • GitHub's API is missing the input validation to catch the error. They've raised an internal issue and will hopefully add the missing input validation, so that trying to create the same invalid comment in the future would return a failure rather than succeeding (and causing a bad user experience on github.com).


Thanks for reporting this, Dmitri -- that does look strange. I agree that looks strange.

So, as far as I can tell -- the reason why that's happening is that the comments were created on an invalid diff line with the index 0. Notice the values of the "position" parameter for those comments in the API response -- they have the value 0. That's not a valid position in the diff -- diff lines start at 1.

So, the problem here is that the API is missing a validation -- the API shouldn't have allowed those comments to be created since they can't be displayed. But due to the missing validation -- the comments were created and exist, but they can't be displayed. This is why the count says that there are two comments, but none are shown.

I hope this explains things, but let me know if it doesn't or you have any other questions. The integration you're using should be able to fix this by making sure comments are created with valid permissions. I'll also open an internal issue so that the team can consider adding more validations there and rejecting calls which don't have a valid position.

dmitshur commented Sep 19, 2017

No problem. GitHub support (h/t @izuzak) got back to me, confirming the following:

  • Comments on a diff line with the index 0 are invalid. Diff lines start at 1. GopherCI should not be creating such invalid comments.

  • GitHub's API is missing the input validation to catch the error. They've raised an internal issue and will hopefully add the missing input validation, so that trying to create the same invalid comment in the future would return a failure rather than succeeding (and causing a bad user experience on github.com).


Thanks for reporting this, Dmitri -- that does look strange. I agree that looks strange.

So, as far as I can tell -- the reason why that's happening is that the comments were created on an invalid diff line with the index 0. Notice the values of the "position" parameter for those comments in the API response -- they have the value 0. That's not a valid position in the diff -- diff lines start at 1.

So, the problem here is that the API is missing a validation -- the API shouldn't have allowed those comments to be created since they can't be displayed. But due to the missing validation -- the comments were created and exist, but they can't be displayed. This is why the count says that there are two comments, but none are shown.

I hope this explains things, but let me know if it doesn't or you have any other questions. The integration you're using should be able to fix this by making sure comments are created with valid permissions. I'll also open an internal issue so that the team can consider adding more validations there and rejecting calls which don't have a valid position.

@bradleyfalzon

This comment has been minimized.

Show comment
Hide comment
@bradleyfalzon

bradleyfalzon Sep 19, 2017

Owner

I agree 100% with GitHub's response and great to see this'll be a validation error too, thanks.

Owner

bradleyfalzon commented Sep 19, 2017

I agree 100% with GitHub's response and great to see this'll be a validation error too, thanks.

@bradleyfalzon

This comment has been minimized.

Show comment
Hide comment
@bradleyfalzon

bradleyfalzon Sep 20, 2017

Owner

Resolved upstream and rebuilt/deployed GopherCI, thanks again for spotting and raising the issue.

Owner

bradleyfalzon commented Sep 20, 2017

Resolved upstream and rebuilt/deployed GopherCI, thanks again for spotting and raising the issue.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 4, 2017

Seeing this issue happen again, both on GopherCI side (it produces an impossible line number value of 0) and GitHub API side (it doesn't prevent an inline comment from being made on an impossible line number, causing a notification to show up, but comment is not visible).

https://gci.gopherci.io/analysis/1247

image

Line 0 is impossible because lines are one-indexed.

I got a notification on GitHub about the comment:

image

But can't see the comment:

shurcooL/users@3500fa3

image

dmitshur commented Dec 4, 2017

Seeing this issue happen again, both on GopherCI side (it produces an impossible line number value of 0) and GitHub API side (it doesn't prevent an inline comment from being made on an impossible line number, causing a notification to show up, but comment is not visible).

https://gci.gopherci.io/analysis/1247

image

Line 0 is impossible because lines are one-indexed.

I got a notification on GitHub about the comment:

image

But can't see the comment:

shurcooL/users@3500fa3

image

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 4, 2017

Worth noting, the output from apicompat in the log states line 49, not 0. It seems it wasn't parsed correctly:

$ apicompat -before 3500fa32fc1c1bce424c831ce06357dd3b2ddab5~1 ./... 245ms
HEAD:users.go:49: breaking change members removed
	type User struct {
		UserSpec
		Elsewhere	[]UserSpec
		Login	string
		Name	string
		Email	string
		AvatarURL	string
		HTMLURL	string
		CreatedAt	time.Time
		UpdatedAt	time.Time
		SiteAdmin	bool
	}
	type User struct {
		UserSpec
		Elsewhere	[]UserSpec
		Login	string
		Name	string
		Email	string
		AvatarURL	string
		HTMLURL	string
		SiteAdmin	bool
	}

dmitshur commented Dec 4, 2017

Worth noting, the output from apicompat in the log states line 49, not 0. It seems it wasn't parsed correctly:

$ apicompat -before 3500fa32fc1c1bce424c831ce06357dd3b2ddab5~1 ./... 245ms
HEAD:users.go:49: breaking change members removed
	type User struct {
		UserSpec
		Elsewhere	[]UserSpec
		Login	string
		Name	string
		Email	string
		AvatarURL	string
		HTMLURL	string
		CreatedAt	time.Time
		UpdatedAt	time.Time
		SiteAdmin	bool
	}
	type User struct {
		UserSpec
		Elsewhere	[]UserSpec
		Login	string
		Name	string
		Email	string
		AvatarURL	string
		HTMLURL	string
		SiteAdmin	bool
	}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment