Skip to content
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

x/build/maintner: GitHubIssue.ClosedBy field is never populated #28745

Open
dmitshur opened this issue Nov 12, 2018 · 4 comments

Comments

@dmitshur
Copy link
Member

commented Nov 12, 2018

There exists a GitHubIssue.ClosedBy field in maintner:

type GitHubIssue struct {
    ...
    ClosedBy *GitHubUser
    ...
}

What did you expect to see?

Accurate values.

What did you see instead?

The field is never populated and always equals to nil for all GitHub issues.

This can be misleading for anyone looking to use that information.

Cause

The closed_by JSON field is documented and shown in the sample response at https://developer.github.com/v3/issues/#get-a-single-issue.

However, maintner uses the https://developer.github.com/v3/issues/#list-issues-for-a-repository endpoint for getting information about many issues at once:

https://github.com/golang/build/blob/23803abc1638efbf100d69fe6d901b14a9ad55fd/maintner/github.go#L1605-L1613

But GitHub doesn't include all detailed fields when listing many issues rather than getting a single issue. The closed_by field is indeed missing:

Response from Get Single Issue Endpoint
  ...
  "comments": 1,
  "created_at": "2018-11-09T00:20:31Z",
  "updated_at": "2018-11-09T00:25:34Z",
  "closed_at": "2018-11-09T00:22:12Z",
  "author_association": "MEMBER",
  "body": "It exists in [...]",
  "closed_by": {
    "login": "gopherbot",
    ...
  }
}

Response from List Issues Endpoint
    ...
    "comments": 1,
    "created_at": "2018-11-09T00:20:31Z",
    "updated_at": "2018-11-09T00:25:34Z",
    "closed_at": "2018-11-09T00:22:12Z",
    "author_association": "MEMBER",
    "body": "It exists in [...]"
  },

Possible Fixes

I see two possible solutions:

  1. Populate the field.
  2. Remove the field (or document it as broken and point to this issue).

Since this field isn't included in the existing endpoint queried by maintner, it would require making additional API calls per issue. That can be extremely expensive and simply not viable.

I'd suggest removing it or documenting, at least as an intermediate step. But open to ideas. /cc @bradfitz

@gopherbot gopherbot added this to the Unreleased milestone Nov 12, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Let's just document it for now with a TODO to this issue.

The first person who needs it can implement.

We have the issue events synced anyway, so the data's in the corpus. We just need to track the oldest close event per issue.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 13, 2018

Change https://golang.org/cl/149238 mentions this issue: maintner: document that GitHubIssue.ClosedBy field is not implemented

@dmitshur

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2018

We have the issue events synced anyway, so the data's in the corpus. We just need to track the oldest close event per issue.

That's a great idea to consider, then no need for extra queries. The close event has additional information like the SHA of the commit that closed the issue (if any): GitHubIssueEvent.CommitID. That can also be useful information to expose.

gopherbot pushed a commit to golang/build that referenced this issue Nov 13, 2018
maintner: document that GitHubIssue.ClosedBy field is not implemented
The ClosedBy field is currently always nil due to the cause described
in the linked issue. Document it with a TODO comment so people don't
need to spend time on figuring that out for themselves.

Updates golang/go#28745

Change-Id: Icaa7b8fd5614dffbfd13a9783b9a71cb87e2af40
Reviewed-on: https://go-review.googlesource.com/c/149238
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@Skarlso

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Hi. Just noting here, that any fix to this, depends on this being fixed: #29396.

Because:

We just need to track the oldest close event per issue.

In same cases apparently that's not recorded. I'm still trying to figure out the why to that. It's somewhere in the chain where the events are recorded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.