Skip to content

Commit

Permalink
correctly list merge-request pipelines
Browse files Browse the repository at this point in the history
The API endpoint seems to require a more precise payload when we attempt
to list merge-request based pipelines.

this should fix #280 and potentially #259
  • Loading branch information
mvisonneau committed Jun 1, 2021
1 parent 85a174b commit 83ca5f2
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ and this project adheres to [0ver](https://0ver.org) (more or less).
### Changed

- Fixed the error handling when comparing 2 refs which resulted into nil pointer dereferences
- Fixed the pulling of merge-request based pipelines
- Bumped all dependencies

## [v0.4.9] - 2021-05-05
Expand Down
10 changes: 9 additions & 1 deletion pkg/exporter/pipelines.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,21 @@ func pullRefMetrics(ref schemas.Ref) error {
}
}

// We need a different syntax if the ref is a merge-request
var refName string
if ref.Kind == schemas.RefKindMergeRequest {
refName = fmt.Sprintf("refs/merge-requests/%s/head", ref.Name)

This comment has been minimized.

Copy link
@mpickering

mpickering Jun 3, 2021

Don't think this is correct still. It should substitute the merge request ID, not the ref.Name (which is the branch name). A more robust way would be to look for the SHA perhaps?

This comment has been minimized.

Copy link
@mvisonneau

mvisonneau Jun 3, 2021

Author Owner

Using the SHA would, unfortunately, prevent us from keeping track of the associated merge request.

I have done some tests locally and can confirm that ref.Name contains the expected content. I reckon the error is somewhere and am looking into it 👀

This comment has been minimized.

Copy link
@mpickering

mpickering Jun 3, 2021

I tested locally and ref.Name contained the branch name but the correct refName required the merge request ID. This was when there was a MR made from another project.

This comment has been minimized.

Copy link
@mpickering

mpickering Jun 3, 2021

In this comment as well, you can see it;s the branch name and not the ID.

#280 (comment)

This comment has been minimized.

Copy link
@mvisonneau

mvisonneau Jun 3, 2021

Author Owner

as I suspected the error was in the payload of the webhook, this change should address the issue: 436f91f

} else {
refName = ref.Name
}

pipelines, err := gitlabClient.GetProjectPipelines(ref.ProjectName, &goGitlab.ListProjectPipelinesOptions{
// We only need the most recent pipeline
ListOptions: goGitlab.ListOptions{
PerPage: 1,
Page: 1,
},
Ref: goGitlab.String(ref.Name),
Ref: &refName,
})
if err != nil {
return fmt.Errorf("error fetching project pipelines for %s: %v", ref.ProjectName, err)
Expand Down
32 changes: 32 additions & 0 deletions pkg/exporter/pipelines_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ func TestPullRefMetricsSucceed(t *testing.T) {

mux.HandleFunc("/api/v4/projects/foo/pipelines",
func(w http.ResponseWriter, r *http.Request) {
assert.Equal(t, "bar", r.URL.Query().Get("ref"))
fmt.Fprint(w, `[{"id":1}]`)
})

Expand Down Expand Up @@ -87,6 +88,37 @@ func TestPullRefMetricsSucceed(t *testing.T) {
}

func TestPullRefMetricsMergeRequestPipeline(t *testing.T) {
resetGlobalValues()
mux, server := configureMockedGitlabClient()
defer server.Close()

mux.HandleFunc("/api/v4/projects/foo/pipelines",
func(w http.ResponseWriter, r *http.Request) {
assert.Equal(t, "refs/merge-requests/1234/head", r.URL.Query().Get("ref"))
fmt.Fprint(w, `[{"id":1}]`)
})

mux.HandleFunc("/api/v4/projects/foo/pipelines/1",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, `{"id":1,"updated_at":"2016-08-11T11:28:34.085Z","duration":300,"status":"running","coverage":"30.2"}`)
})

mux.HandleFunc(fmt.Sprintf("/api/v4/projects/foo/pipelines/1/variables"),
func(w http.ResponseWriter, r *http.Request) {
assert.Equal(t, "GET", r.Method)
fmt.Fprint(w, `[{"key":"foo","value":"bar"}]`)
})

// Metrics pull shall succeed
assert.NoError(t, pullRefMetrics(schemas.Ref{
Kind: schemas.RefKindMergeRequest,
ProjectName: "foo",
Name: "1234",
PullPipelineVariablesEnabled: true,
}))
}

func TestPullRefMetricsMergeRequestPipelineAlreadyLoaded(t *testing.T) {
resetGlobalValues()
ref := schemas.Ref{
Kind: schemas.RefKindMergeRequest,
Expand Down

0 comments on commit 83ca5f2

Please sign in to comment.