Skip to content

Commit de0dba9

Browse files
committed
fix: address PR review feedback
- Use CI_MERGE_REQUEST_PROJECT_PATH for fork-based GitLab MRs, falling back to CI_PROJECT_PATH - Add v1.0 schema backward compatibility tests - Add integration test for GitLab MR metadata with reviewers via API Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
1 parent 8b87add commit de0dba9

4 files changed

Lines changed: 107 additions & 3 deletions

File tree

internal/prinfo/prinfo_test.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,3 +184,49 @@ func TestValidatePRInfo(t *testing.T) {
184184
})
185185
}
186186
}
187+
188+
func TestValidatePRInfoV1_0BackwardCompat(t *testing.T) {
189+
testCases := []struct {
190+
name string
191+
data string
192+
wantErr bool
193+
}{
194+
{
195+
name: "v1.0 valid PR without reviewers",
196+
data: `{
197+
"platform": "github",
198+
"type": "pull_request",
199+
"number": "123",
200+
"url": "https://github.com/owner/repo/pull/123",
201+
"author": "username"
202+
}`,
203+
wantErr: false,
204+
},
205+
{
206+
name: "v1.0 rejects reviewers field",
207+
data: `{
208+
"platform": "github",
209+
"type": "pull_request",
210+
"number": "123",
211+
"url": "https://github.com/owner/repo/pull/123",
212+
"reviewers": [{"login": "reviewer1", "type": "User"}]
213+
}`,
214+
wantErr: true,
215+
},
216+
}
217+
218+
for _, tc := range testCases {
219+
t.Run(tc.name, func(t *testing.T) {
220+
var data interface{}
221+
err := json.Unmarshal([]byte(tc.data), &data)
222+
require.NoError(t, err)
223+
224+
err = schemavalidators.ValidatePRInfo(data, schemavalidators.PRInfoVersion1_0)
225+
if tc.wantErr {
226+
assert.Error(t, err)
227+
} else {
228+
assert.NoError(t, err)
229+
}
230+
})
231+
}
232+
}

pkg/attestation/crafter/materials/chainloop_pr_info_test.go

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
//
2-
// Copyright 2025 The Chainloop Authors.
2+
// Copyright 2025-2026 The Chainloop Authors.
33
//
44
// Licensed under the Apache License, Version 2.0 (the "License");
55
// you may not use this file except in compliance with the License.
@@ -129,6 +129,27 @@ func TestChainloopPRInfoCrafter_Validation(t *testing.T) {
129129
}
130130
}
131131

132+
func TestChainloopPRInfoCrafter_V1_0BackwardCompat(t *testing.T) {
133+
// Data without reviewers should still validate against v1.0
134+
data := prinfo.Data{
135+
Platform: "github",
136+
Type: "pull_request",
137+
Number: "123",
138+
URL: "https://github.com/org/repo/pull/123",
139+
Author: "testuser",
140+
}
141+
142+
dataBytes, err := json.Marshal(data)
143+
require.NoError(t, err)
144+
145+
var rawData interface{}
146+
err = json.Unmarshal(dataBytes, &rawData)
147+
require.NoError(t, err)
148+
149+
err = schemavalidators.ValidatePRInfo(rawData, schemavalidators.PRInfoVersion1_0)
150+
require.NoError(t, err)
151+
}
152+
132153
func TestChainloopPRInfoCrafter_InvalidJSON(t *testing.T) {
133154
tmpDir := t.TempDir()
134155
tmpFile := filepath.Join(tmpDir, "invalid.json")

pkg/attestation/crafter/prmetadata.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,13 @@ func extractGitLabMRMetadata(ctx context.Context, envVars map[string]string) (bo
163163
projectURL := envVars["CI_MERGE_REQUEST_PROJECT_URL"]
164164
mrURL := fmt.Sprintf("%s/-/merge_requests/%s", projectURL, mrIID)
165165

166-
// Fetch reviewers from GitLab API (best-effort)
167-
reviewers := fetchGitLabReviewers(ctx, os.Getenv("CI_SERVER_URL"), os.Getenv("CI_PROJECT_PATH"), mrIID, os.Getenv("CI_JOB_TOKEN"))
166+
// Fetch reviewers from GitLab API (best-effort).
167+
// Prefer CI_MERGE_REQUEST_PROJECT_PATH for fork-based MRs where CI_PROJECT_PATH points to the fork.
168+
projectPath := os.Getenv("CI_MERGE_REQUEST_PROJECT_PATH")
169+
if projectPath == "" {
170+
projectPath = os.Getenv("CI_PROJECT_PATH")
171+
}
172+
reviewers := fetchGitLabReviewers(ctx, os.Getenv("CI_SERVER_URL"), projectPath, mrIID, os.Getenv("CI_JOB_TOKEN"))
168173

169174
metadata := &PRMetadata{
170175
Platform: "gitlab",

pkg/attestation/crafter/prmetadata_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,3 +304,35 @@ func TestFetchGitLabReviewersRequestPath(t *testing.T) {
304304

305305
fetchGitLabReviewers(context.Background(), server.URL, "group/project", "42", "token")
306306
}
307+
308+
func TestExtractGitLabMRMetadataWithReviewers(t *testing.T) {
309+
// Set up a mock GitLab API that returns reviewers
310+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
311+
w.Header().Set("Content-Type", "application/json")
312+
fmt.Fprint(w, `{"reviewers": [{"username": "alice"}, {"username": "coderabbitai"}]}`)
313+
}))
314+
defer server.Close()
315+
316+
// Set env vars that extractGitLabMRMetadata reads via os.Getenv for the API call
317+
t.Setenv("CI_SERVER_URL", server.URL)
318+
t.Setenv("CI_MERGE_REQUEST_PROJECT_PATH", "group/project")
319+
t.Setenv("CI_JOB_TOKEN", "test-token")
320+
321+
envVars := map[string]string{
322+
"CI_PIPELINE_SOURCE": "merge_request_event",
323+
"CI_MERGE_REQUEST_IID": "10",
324+
"CI_MERGE_REQUEST_TITLE": "MR with reviewers",
325+
"CI_MERGE_REQUEST_PROJECT_URL": "https://gitlab.com/group/project",
326+
"GITLAB_USER_LOGIN": "author",
327+
"CI_MERGE_REQUEST_SOURCE_BRANCH_NAME": "feature",
328+
"CI_MERGE_REQUEST_TARGET_BRANCH_NAME": "main",
329+
}
330+
331+
isMR, metadata, err := extractGitLabMRMetadata(context.Background(), envVars)
332+
require.NoError(t, err)
333+
require.True(t, isMR)
334+
require.Len(t, metadata.Reviewers, 2)
335+
assert.Equal(t, "alice", metadata.Reviewers[0].Login)
336+
assert.Equal(t, "unknown", metadata.Reviewers[0].Type)
337+
assert.Equal(t, "coderabbitai", metadata.Reviewers[1].Login)
338+
}

0 commit comments

Comments
 (0)