Repository: openshift-pipelines/pipelines-as-code
Pull request #260 (2021-10-05 14:00:24)

miscies
- When we could not get the token then let the user know
- Set error message as markdown
- GetCommitInfo ASAP in the Run
- Move ACL operations to the acl package for common



Patch:
diff --git a/pkg/acl/acl.go b/pkg/acl/acl.go
new file mode 100644
index 00000000..38c940c6
--- /dev/null
+++ b/pkg/acl/acl.go
@@ -0,0 +1,37 @@
+package acl
+
+import (
+	"regexp"
+
+	"sigs.k8s.io/yaml"
+)
+
+const OKToTestCommentRegexp = `(^|\n)/ok-to-test(\r\n|$)`
+
+// ownersConfig prow owner, only supporting approvers or reviewers in yaml
+type ownersConfig struct {
+	Approvers []string `json:"approvers,omitempty"`
+	Reviewers []string `json:"reviewers,omitempty"`
+}
+
+// Parse a Prow type Owner, Approver files and return true if the sender is in
+// there.
+func UserInOwnerFile(ownerContent string, sender string) (bool, error) {
+	oc := ownersConfig{}
+	err := yaml.Unmarshal([]byte(ownerContent), &oc)
+	if err != nil {
+		return false, err
+	}
+
+	for _, owner := range append(oc.Approvers, oc.Reviewers...) {
+		if owner == sender {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
+func MatchRegexp(reg, comment string) bool {
+	re := regexp.MustCompile(reg)
+	return string(re.Find([]byte(comment))) != ""
+}
diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go
index fcad5cff..60850b53 100644
--- a/pkg/pipelineascode/pipelineascode.go
+++ b/pkg/pipelineascode/pipelineascode.go
@@ -45,6 +45,7 @@ func Run(ctx context.Context, cs *params.Run, vcsintf webvcs.Interface, k8int ku
 
 		if cs.Info.Pac.VCSToken == "" {
 			cs.Clients.Log.Error(msg)
+			cs.Clients.Log.Error("cannot set status since not token has been set")
 			return nil
 		}
 
@@ -71,6 +72,12 @@ func Run(ctx context.Context, cs *params.Run, vcsintf webvcs.Interface, k8int ku
 		vcsintf.SetClient(ctx, cs.Info.Pac)
 	}
 
+	// Get the SHA commit info, we want to get the URL and commit title
+	err = vcsintf.GetCommitInfo(ctx, cs.Info.Event)
+	if err != nil {
+		return err
+	}
+
 	// Check if the submitter is allowed to run this.
 	allowed, err := vcsintf.IsAllowed(ctx, cs.Info.Event)
 	if err != nil {
@@ -94,7 +101,7 @@ func Run(ctx context.Context, cs *params.Run, vcsintf webvcs.Interface, k8int ku
 	// Get everything in tekton directory
 	allTemplates, err := vcsintf.GetTektonDir(ctx, cs.Info.Event, tektonDir)
 	if allTemplates == "" || err != nil {
-		msg := "😿 Could not find a <b>.tekton/</b> directory for this repository"
+		msg := fmt.Sprintf("%s - Could not find a **.tekton/** directory for this repository", cs.Info.Pac.ApplicationName)
 
 		err = createStatus(ctx, vcsintf, cs, webvcs.StatusOpts{
 			Status:     "completed",
@@ -150,12 +157,6 @@ func Run(ctx context.Context, cs *params.Run, vcsintf webvcs.Interface, k8int ku
 		}
 	}
 
-	// Get the SHA commit info, we want to get the URL and commit title
-	err = vcsintf.GetCommitInfo(ctx, cs.Info.Event)
-	if err != nil {
-		return err
-	}
-
 	// Add labels and annotations to pipelinerun
 	addLabelsAndAnnotations(cs, pipelineRun, repo)
 
@@ -176,7 +177,7 @@ func Run(ctx context.Context, cs *params.Run, vcsintf webvcs.Interface, k8int ku
 	msg := fmt.Sprintf(startingPipelineRunText, pr.GetName(), repo.GetNamespace(), repo.GetNamespace(), pr.GetName())
 	err = createStatus(ctx, vcsintf, cs, webvcs.StatusOpts{
 		Status:     "in_progress",
-		Conclusion: "",
+		Conclusion: "pending",
 		Text:       msg,
 		DetailsURL: consoleURL,
 	}, false)
@@ -244,21 +245,26 @@ func Run(ctx context.Context, cs *params.Run, vcsintf webvcs.Interface, k8int ku
 	return err
 }
 
+// cleanupLabel k8s do not like slash in labels value and on push we have the
+// full ref, we replace the "/" by "-". The tools probably need to be aware of
+// it when querying.
+func cleanupLabel(s string) string {
+	replasoeur := strings.NewReplacer("/", "-", " ", "_")
+	return replasoeur.Replace(s)
+}
+
 func addLabelsAndAnnotations(cs *params.Run, pipelineRun *tektonv1beta1.PipelineRun, repo *apipac.Repository) {
 	// Add labels on the soon to be created pipelinerun so UI/CLI can easily
-	// query them. Since K8s do not like slash in labels value and on push we
-	// have the full ref, we replace the "/" by "-". The tools probably need to
-	// be aware of it when querying.
-	refTomakeK8Happy := strings.ReplaceAll(cs.Info.Event.BaseBranch, "/", "-")
+	// query them.
 	pipelineRun.Labels = map[string]string{
 		"app.kubernetes.io/managed-by":              "pipelines-as-code",
-		"pipelinesascode.tekton.dev/url-org":        cs.Info.Event.Owner,
-		"pipelinesascode.tekton.dev/url-repository": cs.Info.Event.Repository,
-		"pipelinesascode.tekton.dev/sha":            cs.Info.Event.SHA,
-		"pipelinesascode.tekton.dev/sender":         cs.Info.Event.Sender,
-		"pipelinesascode.tekton.dev/event-type":     cs.Info.Event.EventType,
-		"pipelinesascode.tekton.dev/branch":         refTomakeK8Happy,
-		"pipelinesascode.tekton.dev/repository":     repo.GetName(),
+		"pipelinesascode.tekton.dev/url-org":        cleanupLabel(cs.Info.Event.Owner),
+		"pipelinesascode.tekton.dev/url-repository": cleanupLabel(cs.Info.Event.Repository),
+		"pipelinesascode.tekton.dev/sha":            cleanupLabel(cs.Info.Event.SHA),
+		"pipelinesascode.tekton.dev/sender":         cleanupLabel(cs.Info.Event.Sender),
+		"pipelinesascode.tekton.dev/event-type":     cleanupLabel(cs.Info.Event.EventType),
+		"pipelinesascode.tekton.dev/branch":         cleanupLabel(cs.Info.Event.BaseBranch),
+		"pipelinesascode.tekton.dev/repository":     cleanupLabel(repo.GetName()),
 	}
 
 	pipelineRun.Annotations["pipelinesascode.tekton.dev/sha-title"] = cs.Info.Event.SHATitle
diff --git a/pkg/webvcs/github/acl.go b/pkg/webvcs/github/acl.go
index ee6b12c5..16714155 100644
--- a/pkg/webvcs/github/acl.go
+++ b/pkg/webvcs/github/acl.go
@@ -4,16 +4,13 @@ import (
 	"context"
 	"fmt"
 	"net/http"
-	"regexp"
 	"strings"
 
 	"github.com/google/go-github/v35/github"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/acl"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
-	"sigs.k8s.io/yaml"
 )
 
-var okToTestCommentRegexp = `(^|\n)/ok-to-test(\r\n|$)`
-
 func (v VCS) IsAllowed(ctx context.Context, event *info.Event) (bool, error) {
 	// Do most of the checks first, if user is a owner or in a organisation
 	allowed, err := v.aclCheckAll(ctx, event)
@@ -49,7 +46,7 @@ func (v VCS) aclAllowedOkToTestFromAnOwner(ctx context.Context, event *info.Even
 		return false, nil
 	}
 
-	comments, err := v.GetStringPullRequestComment(ctx, revent, okToTestCommentRegexp)
+	comments, err := v.GetStringPullRequestComment(ctx, revent, acl.OKToTestCommentRegexp)
 	if err != nil {
 		return false, err
 	}
@@ -67,12 +64,6 @@ func (v VCS) aclAllowedOkToTestFromAnOwner(ctx context.Context, event *info.Even
 	return false, nil
 }
 
-// ownersConfig prow owner, only supporting approvers or reviewers in yaml
-type ownersConfig struct {
-	Approvers []string `json:"approvers,omitempty"`
-	Reviewers []string `json:"reviewers,omitempty"`
-}
-
 // aclCheck check if we are allowed to run the pipeline on that PR
 func (v VCS) aclCheckAll(ctx context.Context, rev *info.Event) (bool, error) {
 	if rev.Owner == rev.Sender {
@@ -92,25 +83,16 @@ func (v VCS) aclCheckAll(ctx context.Context, rev *info.Event) (bool, error) {
 
 	// If we have a prow OWNERS file in the defaultBranch (ie: master) then
 	// parse it in approvers and reviewers field and check if sender is in there.
-	ownerFile, err := v.getFileFromDefaultBranch(ctx, "OWNERS", rev)
-
-	// Don't error out if the OWNERS file cannot be found
-	if err != nil && !strings.Contains(err.Error(), "cannot find") {
-		return false, err
-	} else if ownerFile != "" {
-		var ownerConfig ownersConfig
-		err := yaml.Unmarshal([]byte(ownerFile), &ownerConfig)
-		if err != nil {
-			return false, err
-		}
-		for _, owner := range append(ownerConfig.Approvers, ownerConfig.Reviewers...) {
-			if owner == rev.Sender {
-				return true, nil
-			}
+	ownerContent, err := v.getFileFromDefaultBranch(ctx, "OWNERS", rev)
+	if err != nil {
+		if strings.Contains(err.Error(), "cannot find") {
+			// no owner file, skipping
+			return false, nil
 		}
+		return false, err
 	}
 
-	return false, nil
+	return acl.UserInOwnerFile(ownerContent, rev.Sender)
 }
 
 // checkSenderOrgMembership Get sender user's organization. We can
@@ -155,15 +137,14 @@ func (v VCS) GetStringPullRequestComment(ctx context.Context, runevent *info.Eve
 	if err != nil {
 		return nil, err
 	}
+
 	comments, _, err := v.Client.Issues.ListComments(ctx, runevent.Owner, runevent.Repository,
 		prNumber, &github.IssueListCommentsOptions{})
 	if err != nil {
 		return nil, err
 	}
-
-	re := regexp.MustCompile(reg)
 	for _, v := range comments {
-		if string(re.Find([]byte(v.GetBody()))) != "" {
+		if acl.MatchRegexp(reg, v.GetBody()) {
 			ret = append(ret, v)
 		}
 	}


Test patch:
diff --git a/pkg/acl/acl_test.go b/pkg/acl/acl_test.go
new file mode 100644
index 00000000..a7fe464e
--- /dev/null
+++ b/pkg/acl/acl_test.go
@@ -0,0 +1,88 @@
+package acl
+
+import "testing"
+
+func TestUserInOwnerFile(t *testing.T) {
+	type args struct {
+		ownerContent string
+		sender       string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    bool
+		wantErr bool
+	}{
+		{
+			name: "user in owner file",
+			args: args{
+				ownerContent: "---\n approvers:\n  - allowed\n",
+				sender:       "allowed",
+			},
+			want: true,
+		},
+		{
+			name: "user not in owner file",
+			args: args{
+				ownerContent: "---\n approvers:\n  - allowed\n",
+				sender:       "notallowed",
+			},
+			want: false,
+		},
+		{
+			name: "bad yaml file",
+			args: args{
+				ownerContent: "bad",
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := UserInOwnerFile(tt.args.ownerContent, tt.args.sender)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("UserInOwnerFile() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.want {
+				t.Errorf("UserInOwnerFile() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestMatchRegexp(t *testing.T) {
+	type args struct {
+		reg     string
+		comment string
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "match",
+			args: args{
+				reg:     ".*",
+				comment: "hello",
+			},
+			want: true,
+		},
+		{
+			name: "nomatch",
+			args: args{
+				reg:     "!!!",
+				comment: "foobar",
+			},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := MatchRegexp(tt.args.reg, tt.args.comment); got != tt.want {
+				t.Errorf("MatchRegexp() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}