zyv4yk opened a new pull request, #9087:
URL: https://github.com/apache/devlake/pull/9087
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
### ⚠️ Pre Checklist
- [x] I have read through the [Contributing
Documentation](https://devlake.apache.org/community/).
- [x] I have added relevant tests.
- [ ] I have added relevant documentation.
- [x] I will add labels to the PR, such as `pr-type/bug-fix`,
`pr-type/feature-development`, etc.
### Summary
`_tool_github_pull_requests.merged` is `false` for every pull request
collected through GraphQL, merged ones included.
`convertGithubPullRequest` in
`backend/plugins/github_graphql/tasks/pr_extractor.go` fills `MergedAt`,
`MergedBy` and `MergeCommitSha`, but never `Merged`, so the column keeps its
zero value:
```go
ClosedAt: pull.ClosedAt,
MergedAt: pull.MergedAt,
Body: pull.Body,
```
#8573 closed the same gap on the REST side — it added `Merged: pull.Merged`
to `plugins/github/tasks/pr_extractor.go` — but the GraphQL extractor was not
part of that change, so a connection running on GraphQL still stores the flag
as `false`.
The flag is derived from `mergedAt` rather than collected as a separate
`merged` field, for a practical reason: `mergedAt` is already present in
`_raw_github_graphql_prs` for existing installs, so re-running the extractor
repairs rows that are already collected. Querying `merged` would only fix data
going forward and would need a full re-collect for the rest.
The two are equivalent by construction — GitHub sets `mergedAt` exactly when
a pull request is merged. Checked against the API over the 50 most recent pull
requests in `grafana/grafana` (18 merged): zero rows where `bool(merged) !=
(mergedAt != null)`.
```
gh api graphql -f query='{ repository(owner:"grafana",name:"grafana"){
pullRequests(first:50, states:[MERGED,CLOSED,OPEN],
orderBy:{field:CREATED_AT,direction:DESC}){
nodes { number state merged mergedAt } } } }'
```
### Does this close any open issues?
No open issue. It is the GraphQL half of #8571, which was closed by #8573
for the REST collector only.
### Other Information
Impact is limited today because the domain layer does not read this column
for status: `pr_convertor.go` computes `Status` from `pr.State == "MERGED" ||
(pr.State == "closed" && (pr.Merged || pr.MergedAt != nil))`, and the GraphQL
state is already `MERGED`. Any consumer that reads
`_tool_github_pull_requests.merged` directly, or any future query that keys on
`merged = true`, gets nothing for GraphQL-collected data.
`go build`, `go vet` and `golangci-lint run ./plugins/github_graphql/...`
are clean. `pr_extractor_test.go` covers merged, closed-unmerged and open. The
`e2e` package needs `E2E_DB_URL` and was not run locally; the only
`github_graphql` e2e fixture covers deployments, which this PR does not touch.
I do not have permission to set labels; this is `pr-type/bug-fix`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
