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
Open

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

dmitshur opened this issue Nov 12, 2018 · 4 comments
Labels
Builders x/build issues (builders, bots, dashboards) help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur 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

@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 12, 2018
@gopherbot gopherbot added this to the Unreleased milestone Nov 12, 2018
@bradfitz
Copy link
Contributor

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
Copy link

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

@dmitshur
Copy link
Contributor Author

dmitshur 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.

@dmitshur dmitshur added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 13, 2018
gopherbot pushed a commit to golang/build that referenced this issue Nov 13, 2018
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
Copy link
Contributor

Skarlso 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
Labels
Builders x/build issues (builders, bots, dashboards) help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants