Merge pull request #714 from opendevstack/fix/branch-pattern-case-match

No lowercasing of branch to fix pipeline matching

CHANGELOG.md

@@ -10,6 +10,10 @@ listed in the changelog.
 
 ## [Unreleased]
 
+### Fixed
+
+- ods.yaml branch trigger patterns must be lowercase ([#713](https://github.com/opendevstack/ods-pipeline/issues/713))
+
 ## [0.13.2] - 2023-07-18
 
 ### Fixed

cmd/pipeline-manager/main.go

@@ -195,9 +195,11 @@ func serve() error {
 		Logger:             logger.WithTag("receiver"),
 		BitbucketClient:    bitbucketClient,
 		WebhookSecret:      webhookSecret,
-		Namespace:          namespace,
-		Project:            project,
-		RepoBase:           repoBase,
+		BitbucketWebhookReceiverBase: manager.BitbucketWebhookReceiverBase{
+			Namespace: namespace,
+			Project:   project,
+			RepoBase:  repoBase,
+		},
 	}
 
 	mux := http.NewServeMux()

internal/manager/receive.go

@@ -17,6 +17,15 @@ import (
 
 const changeRefTypeTag = "TAG"
 
+type BitbucketWebhookReceiverBase struct {
+	// Namespace is the Kubernetes namespace in which the server runs.
+	Namespace string
+	// Project is the Bitbucket project to which this server corresponds.
+	Project string
+	// RepoBase is the common URL base of all repositories on Bitbucket.
+	RepoBase string
+}
+
 // BitbucketWebhookReceiver receives webhook requests from Bitbucket.
 type BitbucketWebhookReceiver struct {
 	// Channel to send new runs to
@@ -27,12 +36,8 @@ type BitbucketWebhookReceiver struct {
 	BitbucketClient bitbucketInterface
 	// WebhookSecret is the shared Bitbucket secret to validate webhook requests.
 	WebhookSecret string
-	// Namespace is the Kubernetes namespace in which the server runs.
-	Namespace string
-	// Project is the Bitbucket project to which this server corresponds.
-	Project string
-	// RepoBase is the common URL base of all repositories on Bitbucket.
-	RepoBase string
+
+	BitbucketWebhookReceiverBase
 }
 
 // PipelineInfo holds information about a triggered pipeline.
@@ -67,6 +72,58 @@ func (s *BitbucketWebhookReceiver) Handle(w http.ResponseWriter, r *http.Request
 		)
 	}
 
+	pInfo, err := readBitbucketRequest(&s.BitbucketWebhookReceiverBase, body)
+	if err != nil {
+		return nil, err
+	}
+
+	if len(pInfo.GitSHA) == 0 {
+		csha, err := getCommitSHA(s.BitbucketClient, pInfo.Project, pInfo.Repository, pInfo.GitFullRef)
+		if err != nil {
+			return nil, httpjson.NewInternalProblem("could not get commit SHA", err)
+		}
+		pInfo.GitSHA = csha
+	}
+
+	skip := shouldSkip(s.BitbucketClient, pInfo.Project, pInfo.Repository, pInfo.GitSHA)
+	if skip {
+		return nil, httpjson.NewStatusProblem(
+			http.StatusAccepted, fmt.Sprintf("Commit %s should be skipped", pInfo.GitSHA), nil,
+		)
+	}
+	prInfo, err := extractPullRequestInfo(s.BitbucketClient, pInfo.Project, pInfo.Repository, pInfo.GitSHA)
+	if err != nil {
+		return nil, httpjson.NewInternalProblem("could not extract PR info", err)
+	}
+	pInfo.PullRequestKey = prInfo.ID
+	pInfo.PullRequestBase = prInfo.Base
+
+	odsConfig, err := fetchODSConfig(
+		s.BitbucketClient,
+		pInfo.Project,
+		pInfo.Repository,
+		pInfo.GitFullRef,
+	)
+	if err != nil {
+		return nil, httpjson.NewInternalProblem(
+			fmt.Sprintf("could not fetch ODS config for repo %s", pInfo.Repository), err,
+		)
+	}
+
+	s.Logger.Infof("%+v", pInfo)
+
+	cfg := identifyPipelineConfig(*pInfo, *odsConfig, pInfo.Component)
+	if cfg == nil {
+		return nil, httpjson.NewStatusProblem(
+			http.StatusAccepted, "Could not identify any pipeline to run as no trigger matched", nil,
+		)
+	}
+
+	s.TriggeredPipelines <- *cfg
+	return pInfo, nil
+}
+
+func readBitbucketRequest(bbb *BitbucketWebhookReceiverBase, body []byte) (*PipelineInfo, error) {
 	req := &requestBitbucket{}
 	if err := json.Unmarshal(body, &req); err != nil {
 		return nil, httpjson.NewStatusProblem(
@@ -87,15 +144,15 @@ func (s *BitbucketWebhookReceiver) Handle(w http.ResponseWriter, r *http.Request
 	if req.EventKey == "repo:refs_changed" {
 		repo = strings.ToLower(req.Repository.Slug)
 		change := req.Changes[0]
-		gitRef = strings.ToLower(change.Ref.DisplayID)
+		gitRef = change.Ref.DisplayID
 		gitFullRef = change.Ref.ID
 
 		projectParam = req.Repository.Project.Key
 		commitSHA = change.ToHash
 		changeRefType = change.Ref.Type
 	} else if strings.HasPrefix(req.EventKey, "pr:") {
 		repo = strings.ToLower(req.PullRequest.FromRef.Repository.Slug)
-		gitRef = strings.ToLower(req.PullRequest.FromRef.DisplayID)
+		gitRef = req.PullRequest.FromRef.DisplayID
 		gitFullRef = req.PullRequest.FromRef.ID
 		projectParam = req.PullRequest.FromRef.Repository.Project.Key
 		if req.Comment != nil {
@@ -107,70 +164,26 @@ func (s *BitbucketWebhookReceiver) Handle(w http.ResponseWriter, r *http.Request
 			http.StatusBadRequest, fmt.Sprintf("unsupported event key: %s", req.EventKey), nil,
 		)
 	}
-
-	project = determineProject(s.Project, projectParam)
+	project = determineProject(bbb.Project, projectParam)
 	component = strings.TrimPrefix(repo, project+"-")
+
 	pInfo := PipelineInfo{
 		Project:    project,
 		Component:  component,
 		Repository: repo,
 		GitRef:     gitRef,
 		GitFullRef: gitFullRef,
-		RepoBase:   s.RepoBase,
+		RepoBase:   bbb.RepoBase,
 		// Assemble GitURI from scratch instead of using user-supplied URI to
 		// protect against attacks from external Bitbucket servers and/or projects.
-		GitURI:        fmt.Sprintf("%s/%s/%s.git", s.RepoBase, project, repo),
-		Namespace:     s.Namespace,
+		GitURI:        fmt.Sprintf("%s/%s/%s.git", bbb.RepoBase, project, repo),
+		GitSHA:        commitSHA,
+		Namespace:     bbb.Namespace,
 		TriggerEvent:  req.EventKey,
 		ChangeRefType: changeRefType,
 		Comment:       commentText,
 	}
-
-	if len(commitSHA) == 0 {
-		csha, err := getCommitSHA(s.BitbucketClient, pInfo.Project, pInfo.Repository, pInfo.GitFullRef)
-		if err != nil {
-			return nil, httpjson.NewInternalProblem("could not get commit SHA", err)
-		}
-		commitSHA = csha
-	}
-	pInfo.GitSHA = commitSHA
-
-	skip := shouldSkip(s.BitbucketClient, pInfo.Project, pInfo.Repository, commitSHA)
-	if skip {
-		return nil, httpjson.NewStatusProblem(
-			http.StatusAccepted, fmt.Sprintf("Commit %s should be skipped", commitSHA), nil,
-		)
-	}
-	prInfo, err := extractPullRequestInfo(s.BitbucketClient, pInfo.Project, pInfo.Repository, commitSHA)
-	if err != nil {
-		return nil, httpjson.NewInternalProblem("could not extract PR info", err)
-	}
-	pInfo.PullRequestKey = prInfo.ID
-	pInfo.PullRequestBase = prInfo.Base
-
-	odsConfig, err := fetchODSConfig(
-		s.BitbucketClient,
-		pInfo.Project,
-		pInfo.Repository,
-		pInfo.GitFullRef,
-	)
-	if err != nil {
-		return nil, httpjson.NewInternalProblem(
-			fmt.Sprintf("could not fetch ODS config for repo %s", pInfo.Repository), err,
-		)
-	}
-
-	s.Logger.Infof("%+v", pInfo)
-
-	cfg := identifyPipelineConfig(pInfo, *odsConfig, component)
-	if cfg == nil {
-		return nil, httpjson.NewStatusProblem(
-			http.StatusAccepted, "Could not identify any pipeline to run as no trigger matched", nil,
-		)
-	}
-
-	s.TriggeredPipelines <- *cfg
-	return pInfo, nil
+	return &pInfo, nil
 }
 
 // identifyPipelineConfig finds the first configuration matching the triggering event

internal/manager/receive_test.go

@@ -111,12 +111,14 @@ func TestFetchODSConfig(t *testing.T) {
 func testServer(bc bitbucketInterface, ch chan PipelineConfig) *httptest.Server {
 	r := &BitbucketWebhookReceiver{
 		TriggeredPipelines: ch,
-		Namespace:          "bar-cd",
-		Project:            "bar",
 		WebhookSecret:      testWebhookSecret,
-		RepoBase:           "https://domain.com",
 		BitbucketClient:    bc,
 		Logger:             &logging.LeveledLogger{Level: logging.LevelNull},
+		BitbucketWebhookReceiverBase: BitbucketWebhookReceiverBase{
+			Namespace: "bar-cd",
+			Project:   "bar",
+			RepoBase:  "https://domain.com",
+		},
 	}
 	return httptest.NewServer(BitbucketHandler(r))
 }
@@ -448,6 +450,18 @@ func TestIdentifyPipelineConfig(t *testing.T) {
 			wantPipelineIndex: 0,
 			wantTriggerIndex:  0,
 		},
+		"branch push - pipeline with one trigger with matching branch constraint upper case": {
+			pInfo: PipelineInfo{ChangeRefType: "BRANCH", GitRef: "feature/FOO-123-hello-world"},
+			odsConfig: config.ODS{
+				Pipelines: []config.Pipeline{
+					{Triggers: []config.Trigger{
+						{Branches: []string{"feature/FOO-123-hello-world"}},
+					}},
+				},
+			},
+			wantPipelineIndex: 0,
+			wantTriggerIndex:  0,
+		},
 		"branch push - pipeline with one trigger with non-matching branch constraint": {
 			pInfo: PipelineInfo{ChangeRefType: "BRANCH", GitRef: "develop"},
 			odsConfig: config.ODS{
@@ -704,34 +718,104 @@ func TestIdentifyPipelineConfig(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			// Annotate wanted pipeline/trigger so that
-			// we can check later if it was selected.
-			if tc.wantPipelineIndex > -1 {
-				tc.odsConfig.Pipelines[tc.wantPipelineIndex].Tasks = []v1beta1.PipelineTask{
-					{Name: "match this"},
-				}
-				if tc.wantTriggerIndex > -1 {
-					tc.odsConfig.Pipelines[tc.wantPipelineIndex].Triggers[tc.wantTriggerIndex].Params = []v1beta1.Param{
-						tektonStringParam("match", "this"),
-					}
-				}
-			}
-			got := identifyPipelineConfig(tc.pInfo, tc.odsConfig, "component")
-			if tc.wantPipelineIndex > -1 && got == nil {
-				t.Fatal("wanted a matching pipeline but got none")
+			verifyMatchingPipelineInfo(&tc.pInfo, &tc.odsConfig, tc.wantPipelineIndex, tc.wantTriggerIndex, t)
+		})
+	}
+}
+
+func verifyMatchingPipelineInfo(pInfo *PipelineInfo, odsConfig *config.ODS, wantPipelineIndex int, wantTriggerIndex int, t *testing.T) {
+	if wantPipelineIndex > -1 {
+		// Annotate wanted pipeline/trigger so that
+		// we can check later if it was selected.
+		// no matching pipeline, as wanted by the test case.
+		odsConfig.Pipelines[wantPipelineIndex].Tasks = []v1beta1.PipelineTask{
+			{Name: "match this"},
+		}
+		if wantTriggerIndex > -1 {
+			odsConfig.Pipelines[wantPipelineIndex].Triggers[wantTriggerIndex].Params = []v1beta1.Param{
+				tektonStringParam("match", "this"),
 			}
-			if tc.wantPipelineIndex < 0 && got != nil {
-				t.Fatal("wanted no matching pipeline, but got one")
+		}
+	}
+	got := identifyPipelineConfig(*pInfo, *odsConfig, "component")
+	if wantPipelineIndex > -1 && got == nil {
+		t.Fatal("wanted a matching pipeline but got none")
+	}
+	if wantPipelineIndex < 0 && got != nil {
+		t.Fatal("wanted no matching pipeline, but got one")
+	}
+	if wantPipelineIndex < 0 && got == nil {
+		return // no matching pipeline, as wanted by the test case.
+	}
+	if len(got.PipelineSpec.Tasks) < 1 {
+		t.Fatal("did not match wanted pipeline")
+	}
+	if wantTriggerIndex > -1 && len(got.Params) < 1 {
+		t.Fatal("did not match wanted trigger")
+	}
+}
+
+func TestProcessWebhookPayloadAndIdentifyPipelineConfig(t *testing.T) {
+	tests := map[string]struct {
+		requestBodyFixture string
+		bbb                BitbucketWebhookReceiverBase
+		wantPipelineInfo   PipelineInfo
+		odsConfig          config.ODS
+		// Index of pipeline that should be selected. -1 indicates no pipeline should be selected.
+		wantPipelineIndex int
+		// Index of trigger within pipeline that should be selected. -1 indicates no trigger should be selected.
+		wantTriggerIndex int
+	}{
+		"repo:refs_changed (branch push) identifies pipeline with matching branch": {
+			requestBodyFixture: "manager/payload-feature-branch.json",
+			bbb:                BitbucketWebhookReceiverBase{Namespace: "bar", Project: "foo", RepoBase: "base"},
+			wantPipelineInfo: PipelineInfo{
+				Project:       "foo",
+				Component:     "bar",
+				Repository:    "foo-bar",
+				GitRef:        "feature/FOO-123-hello-world",
+				GitFullRef:    "refs/heads/feature/FOO-123-hello-world",
+				GitSHA:        "0e183aa3bc3c6deb8f40b93fb2fc4354533cf62f",
+				RepoBase:      "base",
+				GitURI:        "base/foo/foo-bar.git",
+				Namespace:     "bar",
+				TriggerEvent:  "repo:refs_changed",
+				ChangeRefType: "BRANCH",
+			},
+			odsConfig: config.ODS{
+				Pipelines: []config.Pipeline{
+					{Triggers: []config.Trigger{
+						{Branches: []string{"feature/FOO-123-hello-world"}},
+					}},
+				},
+			},
+			wantPipelineIndex: 0,
+			wantTriggerIndex:  -1,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			filename := filepath.Join(projectpath.Root, "test/testdata/fixtures", tc.requestBodyFixture)
+			f, err := os.Open(filename)
+			if err != nil {
+				t.Fatal(err)
 			}
-			if tc.wantPipelineIndex < 0 && got == nil {
-				return // no matching pipeline, as wanted by the test case.
+			body, err := io.ReadAll(f)
+			if err != nil {
+				t.Fatal(err)
 			}
-			if len(got.PipelineSpec.Tasks) < 1 {
-				t.Fatal("did not match wanted pipeline")
+			pInfo, err := readBitbucketRequest(&tc.bbb, body)
+			if err != nil {
+				t.Fatal(err)
 			}
-			if tc.wantTriggerIndex > -1 && len(got.Params) < 1 {
-				t.Fatal("did not match wanted trigger")
+			if diff := cmp.Diff(tc.wantPipelineInfo, *pInfo); diff != "" {
+				t.Fatalf("pInfo mismatch (-want, +got):\n%s", diff)
 			}
+
+			// identify pipeline based on pInfo
+
+			verifyMatchingPipelineInfo(pInfo, &tc.odsConfig, tc.wantPipelineIndex, tc.wantTriggerIndex, t)
 		})
 	}
 }