Skip to content

Commit 19ef67e

Browse files
committed
fix(attestation): address PR review feedback on GitHub PR metadata
- Use GITHUB_REPOSITORY_OWNER env var directly for owner instead of splitting GITHUB_REPOSITORY; remove now-unused splitOwnerRepo helper - Add comment clarifying why both event payload and API are used as sources for requested reviewers (event is a snapshot, API is current) - Decode requested_reviewers API response directly into []prinfo.Reviewer instead of an equivalent anonymous struct Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
1 parent 9482cad commit 19ef67e

2 files changed

Lines changed: 19 additions & 35 deletions

File tree

pkg/attestation/crafter/prmetadata.go

Lines changed: 14 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,8 @@ func extractGitHubPRMetadata(ctx context.Context, envVars map[string]string) (bo
121121

122122
// GITHUB_TOKEN is read via os.Getenv to avoid persisting it in the attestation envVars map.
123123
// GITHUB_API_BASE_URL can be overridden (e.g. in tests); defaults to api.github.com.
124-
parts := splitOwnerRepo(envVars["GITHUB_REPOSITORY"])
125-
owner, repo := parts[0], parts[1]
124+
owner := envVars["GITHUB_REPOSITORY_OWNER"]
125+
_, repo, _ := strings.Cut(envVars["GITHUB_REPOSITORY"], "/")
126126
prNumber := fmt.Sprintf("%d", event.PullRequest.Number)
127127
githubAPIBase := os.Getenv("GITHUB_API_BASE_URL")
128128
if githubAPIBase == "" {
@@ -152,6 +152,9 @@ func extractGitHubPRMetadata(ctx context.Context, envVars map[string]string) (bo
152152
})
153153
}
154154

155+
// Also fetch from the API: the event payload is a snapshot at dispatch time, so reviewers
156+
// added after the event fires won't appear in event.PullRequest.RequestedReviewers.
157+
// The API reflects current state and may include those late additions.
155158
for _, r := range fetchGitHubRequestedReviewers(ctx, githubAPIBase, owner, repo, prNumber, token) {
156159
if _, exists := reviewerMap[r.Login]; !exists {
157160
reviewerMap[r.Login] = len(reviewers)
@@ -185,17 +188,6 @@ func extractGitHubPRMetadata(ctx context.Context, envVars map[string]string) (bo
185188
return true, metadata, nil
186189
}
187190

188-
// splitOwnerRepo splits "owner/repo" into [owner, repo].
189-
// Returns ["", ""] if the string does not contain a slash.
190-
func splitOwnerRepo(ownerRepo string) [2]string {
191-
for i := 0; i < len(ownerRepo); i++ {
192-
if ownerRepo[i] == '/' {
193-
return [2]string{ownerRepo[:i], ownerRepo[i+1:]}
194-
}
195-
}
196-
return [2]string{"", ""}
197-
}
198-
199191
// fetchGitHubRequestedReviewers fetches the list of users explicitly requested to review a PR.
200192
// Returns nil on any failure (best-effort).
201193
// baseURL is the GitHub API base (e.g. "https://api.github.com"); can be overridden in tests.
@@ -225,32 +217,23 @@ func fetchGitHubRequestedReviewers(ctx context.Context, baseURL, owner, repo, pr
225217
}
226218

227219
var result struct {
228-
Users []struct {
229-
Login string `json:"login"`
230-
Type string `json:"type"`
231-
} `json:"users"`
220+
Users []prinfo.Reviewer `json:"users"`
232221
}
233222
if err := json.NewDecoder(resp.Body).Decode(&result); err != nil {
234223
return nil
235224
}
236225

237-
var reviewers []prinfo.Reviewer
238-
for _, u := range result.Users {
239-
reviewerType := u.Type
240-
if reviewerType == "" {
241-
reviewerType = "unknown"
242-
}
243-
reviewers = append(reviewers, prinfo.Reviewer{
244-
Login: u.Login,
245-
Type: reviewerType,
246-
Requested: true,
247-
})
226+
if len(result.Users) == 0 {
227+
return nil
248228
}
249229

250-
if len(reviewers) == 0 {
251-
return nil
230+
for i := range result.Users {
231+
result.Users[i].Requested = true
232+
if result.Users[i].Type == "" {
233+
result.Users[i].Type = "unknown"
234+
}
252235
}
253-
return reviewers
236+
return result.Users
254237
}
255238

256239
// fetchGitHubReviews fetches all PR reviews from the GitHub API, following pagination.

pkg/attestation/crafter/prmetadata_test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -642,10 +642,11 @@ func TestExtractGitHubPRMetadataWithAPIReviews(t *testing.T) {
642642
envVars := map[string]string{
643643
"GITHUB_EVENT_NAME": "pull_request",
644644
// github_pr_event.json has requested_reviewers: reviewer1 (User), coderabbitai (Bot)
645-
"GITHUB_EVENT_PATH": filepath.Join("testdata", "github_pr_event.json"),
646-
"GITHUB_HEAD_REF": "feature-branch",
647-
"GITHUB_BASE_REF": "main",
648-
"GITHUB_REPOSITORY": "owner/repo",
645+
"GITHUB_EVENT_PATH": filepath.Join("testdata", "github_pr_event.json"),
646+
"GITHUB_HEAD_REF": "feature-branch",
647+
"GITHUB_BASE_REF": "main",
648+
"GITHUB_REPOSITORY": "owner/repo",
649+
"GITHUB_REPOSITORY_OWNER": "owner",
649650
}
650651

651652
isPR, metadata, err := extractGitHubPRMetadata(context.Background(), envVars)

0 commit comments

Comments
 (0)