diff --git a/tep-notifier/.gitignore b/bots/tep-automation/.gitignore similarity index 100% rename from tep-notifier/.gitignore rename to bots/tep-automation/.gitignore diff --git a/tep-notifier/.ko.yaml b/bots/tep-automation/.ko.yaml similarity index 100% rename from tep-notifier/.ko.yaml rename to bots/tep-automation/.ko.yaml diff --git a/tep-notifier/Makefile b/bots/tep-automation/Makefile similarity index 96% rename from tep-notifier/Makefile rename to bots/tep-automation/Makefile index 64e9d9df87..5ea13c11bc 100644 --- a/tep-notifier/Makefile +++ b/bots/tep-automation/Makefile @@ -1,4 +1,4 @@ -BINARY_NAME = tep-notifier +BINARY_NAME = tep-automation GO = go BIN = $(CURDIR)/.bin @@ -56,7 +56,7 @@ test: test-unit ## run all tests .PHONY: test-unit test-unit: ## run unit tests @echo "Running unit tests..." - @$(GO) test -failfast -v -cover ./... + @$(GO) test -short -cover ./... .PHONY: clean clean: ## clean build artifacts @@ -84,7 +84,7 @@ $(BIN)/goimports: PACKAGE=golang.org/x/tools/cmd/goimports .PHONY: goimports goimports: $(GOIMPORTS) @echo "Running goimports..." - @$(GOIMPORTS) -l -e -w pkg + @$(GOIMPORTS) -l -e -w pkg cmd .PHONY: fmt fmt: goimports diff --git a/tep-notifier/cmd/controller/main.go b/bots/tep-automation/cmd/controller/main.go similarity index 62% rename from tep-notifier/cmd/controller/main.go rename to bots/tep-automation/cmd/controller/main.go index 549f749c62..6d501537e4 100644 --- a/tep-notifier/cmd/controller/main.go +++ b/bots/tep-automation/cmd/controller/main.go @@ -2,10 +2,12 @@ package main import ( "flag" - "github.com/tektoncd/plumbing/tep-notifier/pkg/reconciler" + "log" + + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/performers" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/reconciler" "knative.dev/pkg/injection/sharedmain" "knative.dev/pkg/signals" - "log" ) var ( @@ -18,5 +20,6 @@ func main() { if ghToken == nil { log.Fatal("no github-token specified") } - sharedmain.MainWithContext(signals.NewContext(), reconciler.ControllerName, reconciler.NewController(*ghToken)) + + sharedmain.MainWithContext(signals.NewContext(), reconciler.ControllerName, reconciler.NewController(*ghToken, performers.NewPRNotifierAsPerformer)) } diff --git a/tep-notifier/config/200-serviceaccount.yaml b/bots/tep-automation/config/200-serviceaccount.yaml similarity index 100% rename from tep-notifier/config/200-serviceaccount.yaml rename to bots/tep-automation/config/200-serviceaccount.yaml diff --git a/tep-notifier/config/201-clusterrole.yaml b/bots/tep-automation/config/201-clusterrole.yaml similarity index 100% rename from tep-notifier/config/201-clusterrole.yaml rename to bots/tep-automation/config/201-clusterrole.yaml diff --git a/tep-notifier/config/201-role.yaml b/bots/tep-automation/config/201-role.yaml similarity index 100% rename from tep-notifier/config/201-role.yaml rename to bots/tep-automation/config/201-role.yaml diff --git a/tep-notifier/config/201-rolebinding.yaml b/bots/tep-automation/config/201-rolebinding.yaml similarity index 100% rename from tep-notifier/config/201-rolebinding.yaml rename to bots/tep-automation/config/201-rolebinding.yaml diff --git a/tep-notifier/config/202-clusterrolebinding.yaml b/bots/tep-automation/config/202-clusterrolebinding.yaml similarity index 100% rename from tep-notifier/config/202-clusterrolebinding.yaml rename to bots/tep-automation/config/202-clusterrolebinding.yaml diff --git a/tep-notifier/config/500-controller.yaml b/bots/tep-automation/config/500-controller.yaml similarity index 100% rename from tep-notifier/config/500-controller.yaml rename to bots/tep-automation/config/500-controller.yaml diff --git a/tep-notifier/go.mod b/bots/tep-automation/go.mod similarity index 95% rename from tep-notifier/go.mod rename to bots/tep-automation/go.mod index af501e7330..9c3db31285 100644 --- a/tep-notifier/go.mod +++ b/bots/tep-automation/go.mod @@ -1,14 +1,19 @@ -module github.com/tektoncd/plumbing/tep-notifier +module github.com/tektoncd/plumbing/bots/tep-automation go 1.17 require ( github.com/google/go-cmp v0.5.6 github.com/google/go-github/v41 v41.0.0 + github.com/hashicorp/go-multierror v1.1.1 github.com/pkg/errors v0.9.1 github.com/stretchr/testify v1.7.0 github.com/tektoncd/pipeline v0.31.0 + github.com/yuin/goldmark v1.4.4 + github.com/yuin/goldmark-meta v1.0.0 golang.org/x/oauth2 v0.0.0-20211005180243-6b3c2da341f1 + k8s.io/api v0.21.4 + k8s.io/apimachinery v0.21.4 k8s.io/client-go v0.21.4 knative.dev/pkg v0.0.0-20211101212339-96c0204a70dc ) @@ -44,7 +49,6 @@ require ( github.com/googleapis/gnostic v0.5.3 // indirect github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect github.com/hashicorp/errwrap v1.0.0 // indirect - github.com/hashicorp/go-multierror v1.1.1 // indirect github.com/hashicorp/golang-lru v0.5.4 // indirect github.com/imdario/mergo v0.3.11 // indirect github.com/josharian/intern v1.0.0 // indirect @@ -82,9 +86,7 @@ require ( gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect - k8s.io/api v0.21.4 // indirect k8s.io/apiextensions-apiserver v0.21.4 // indirect - k8s.io/apimachinery v0.21.4 // indirect k8s.io/klog v1.0.0 // indirect k8s.io/klog/v2 v2.8.0 // indirect k8s.io/kube-openapi v0.0.0-20210305001622-591a79e4bda7 // indirect diff --git a/tep-notifier/go.sum b/bots/tep-automation/go.sum similarity index 99% rename from tep-notifier/go.sum rename to bots/tep-automation/go.sum index 521fd0ce21..72a55d6d19 100644 --- a/tep-notifier/go.sum +++ b/bots/tep-automation/go.sum @@ -910,6 +910,10 @@ github.com/yuin/goldmark v1.1.32/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9de github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k= github.com/yuin/goldmark v1.4.0/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k= +github.com/yuin/goldmark v1.4.4 h1:zNWRjYUW32G9KirMXYHQHVNFkXvMI7LpgNW2AgYAoIs= +github.com/yuin/goldmark v1.4.4/go.mod h1:rmuwmfZ0+bvzB24eSC//bk1R1Zp3hM0OXYv/G2LIilg= +github.com/yuin/goldmark-meta v1.0.0 h1:ScsatUIT2gFS6azqzLGUjgOnELsBOxMXerM3ogdJhAM= +github.com/yuin/goldmark-meta v1.0.0/go.mod h1:zsNNOrZ4nLuyHAJeLQEZcQat8dm70SmB2kHbls092Gc= github.com/yvasiyarov/go-metrics v0.0.0-20140926110328-57bccd1ccd43/go.mod h1:aX5oPXxHm3bOH+xeAttToC8pqch2ScQN/JoXYupl6xs= github.com/yvasiyarov/gorelic v0.0.0-20141212073537-a9bba5b9ab50/go.mod h1:NUSPSUX/bi6SeDMUh6brw0nXpxHnc96TguQh0+r/ssA= github.com/yvasiyarov/newrelic_platform_go v0.0.0-20140908184405-b21fdbd4370f/go.mod h1:GlGEuHIJweS1mbCqG+7vt2nvWLzLLnRHbXz5JKd/Qbg= diff --git a/bots/tep-automation/pkg/ghclient/ghclient.go b/bots/tep-automation/pkg/ghclient/ghclient.go new file mode 100644 index 0000000000..aedfee62b5 --- /dev/null +++ b/bots/tep-automation/pkg/ghclient/ghclient.go @@ -0,0 +1,353 @@ +package ghclient + +import ( + "context" + "fmt" + "path/filepath" + "regexp" + "strings" + + "github.com/google/go-github/v41/github" + "github.com/pkg/errors" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" + "golang.org/x/oauth2" +) + +const ( + // TEPsOwner is the GitHub owner for the repo containing the TEPs. + TEPsOwner = "tektoncd" + // TEPsRepo is the GitHub repository containing the TEPs, under TEPsOwner. + TEPsRepo = "community" + // TEPsDirectory is the directory containing the TEPs, within TEPsRepo. + TEPsDirectory = "teps" + // TEPsBranch is the branch in TEPsRepo we will look at. + TEPsBranch = "main" + // TEPsReadmeFile is the filename to find the list of TEPs and statuses in. + TEPsReadmeFile = "README.md" + + // BotUser is the user we will be making comments with + BotUser = "tekton-robot" + + // TrackingIssueLabel is a label put on tracking issues to identify them as such. + TrackingIssueLabel = "tep-tracking" + + // TrackingIssueTitleFmt is used to construct TEP tracking issue titles + TrackingIssueTitleFmt = "TEP-%s Tracking Issue" +) + +var ( + tepPRFilenameRegex = regexp.MustCompile(`^teps/(\d+)-.*\.md$`) + trackingIssueTitleRegex = regexp.MustCompile(`^TEP-(\d+) Tracking Issue$`) +) + +// TEPGHClient is a wrapper around github.Client that exposes functions needed elsewhere in this tooling. +type TEPGHClient struct { + client *github.Client +} + +// NewTEPGHClientFromToken creates a new client using the given token +func NewTEPGHClientFromToken(ctx context.Context, ghToken string) *TEPGHClient { + // Allow anonymous for testing purposes + if ghToken == "" { + return &TEPGHClient{ + client: github.NewClient(nil), + } + } + + ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: ghToken}) + tc := oauth2.NewClient(ctx, ts) + + return &TEPGHClient{ + client: github.NewClient(tc), + } +} + +// NewTEPGHClient creates a new client using the given *github.Client +func NewTEPGHClient(ghClient *github.Client) *TEPGHClient { + return &TEPGHClient{client: ghClient} +} + +// GetTEPsFromReadme fetches https://github.com/tektoncd/community/blob/main/teps/README.md, parses out the TEPs from +// the table in that file, and returns a map of TEP IDs (i.e., "1234") to TEPInfo for that TEP. +func (tgc *TEPGHClient) GetTEPsFromReadme(ctx context.Context) (map[string]tep.TEPInfo, error) { + fc, _, _, err := tgc.client.Repositories.GetContents(ctx, TEPsOwner, TEPsRepo, filepath.Join(TEPsDirectory, TEPsReadmeFile), &github.RepositoryContentGetOptions{ + Ref: TEPsBranch, + }) + if err != nil { + return nil, errors.Wrapf(err, "fetching contents of https://github.com/%s/%s/blob/%s/%s/%s", TEPsOwner, TEPsRepo, + TEPsBranch, TEPsDirectory, TEPsReadmeFile) + } + + readmeStr, err := fc.GetContent() + if err != nil { + return nil, errors.Wrapf(err, "reading content of https://github.com/%s/%s/blob/%s/%s/%s", TEPsOwner, TEPsRepo, + TEPsBranch, TEPsDirectory, TEPsReadmeFile) + } + + teps, err := tep.ExtractTEPsFromReadme(readmeStr) + if err != nil { + return nil, errors.Wrapf(err, "parsing content of https://github.com/%s/%s/blob/%s/%s/%s", TEPsOwner, TEPsRepo, + TEPsBranch, TEPsDirectory, TEPsReadmeFile) + } + + return teps, nil +} + +// TEPCommentsOnPR finds all comments on the given PR made by this notifier +func (tgc *TEPGHClient) TEPCommentsOnPR(ctx context.Context, repo string, prNumber int) ([]tep.CommentInfo, error) { + listOpts := &github.IssueListCommentsOptions{ + ListOptions: github.ListOptions{ + PerPage: 20, + }, + } + + var tepComments []tep.CommentInfo + + for { + comments, resp, err := tgc.client.Issues.ListComments(ctx, TEPsOwner, repo, prNumber, listOpts) + if err != nil { + return nil, errors.Wrapf(err, "getting comments for PR #%d in %s/%s", prNumber, TEPsOwner, repo) + } + + for _, c := range comments { + if c.ID != nil && c.Body != nil && c.User != nil && c.User.Login != nil && *c.User.Login == BotUser { + tepsOnComment, toImplemented := tep.GetTEPCommentDetails(*c.Body) + + if len(tepsOnComment) > 0 { + tci := tep.CommentInfo{ + CommentID: *c.ID, + TEPs: []string{}, + ToImplemented: toImplemented, + } + for tID, tStatus := range tepsOnComment { + if !tep.IsValidStatus(tStatus) { + return nil, fmt.Errorf("metadata for TEP-%s has invalid status %s", tID, tStatus) + } + tci.TEPs = append(tci.TEPs, tID) + } + tepComments = append(tepComments, tci) + } + } + } + + if resp.NextPage == 0 { + break + } + listOpts.Page = resp.NextPage + } + + return tepComments, nil +} + +// AddComment adds a new comment to the PR +func (tgc *TEPGHClient) AddComment(ctx context.Context, repo string, prNumber int, body string) error { + input := &github.IssueComment{ + Body: github.String(body), + } + _, _, err := tgc.client.Issues.CreateComment(ctx, TEPsOwner, repo, prNumber, input) + return err +} + +// EditComment updates an existing comment on the PR +func (tgc *TEPGHClient) EditComment(ctx context.Context, repo string, commentID int64, body string) error { + input := &github.IssueComment{ + Body: github.String(body), + } + _, _, err := tgc.client.Issues.EditComment(ctx, TEPsOwner, repo, commentID, input) + return err +} + +// CreateTrackingIssue creates a tracking issue for a TEP in the community repo +func (tgc *TEPGHClient) CreateTrackingIssue(ctx context.Context, id, comment string, authors []string, status tep.Status) error { + labels := []string{ + TrackingIssueLabel, + status.TrackingLabel(), + } + + var assignees []string + assignees = append(assignees, authors...) + + input := &github.IssueRequest{ + Title: github.String(fmt.Sprintf(TrackingIssueTitleFmt, id)), + Body: github.String(comment), + Labels: &labels, + Assignees: &assignees, + } + + _, _, err := tgc.client.Issues.Create(ctx, TEPsOwner, TEPsRepo, input) + return err +} + +// UpdateTrackingIssue updates a tracking issue for a TEP in the community repo +func (tgc *TEPGHClient) UpdateTrackingIssue(ctx context.Context, issueNumber int, id, comment string, authors []string, status tep.Status) error { + labels := []string{ + TrackingIssueLabel, + status.TrackingLabel(), + } + + var assignees []string + assignees = append(assignees, authors...) + + input := &github.IssueRequest{ + Title: github.String(fmt.Sprintf(TrackingIssueTitleFmt, id)), + Body: github.String(comment), + Labels: &labels, + Assignees: &assignees, + } + + _, _, err := tgc.client.Issues.Edit(ctx, TEPsOwner, TEPsRepo, issueNumber, input) + return err +} + +// ExtractTEPInfoFromTEPPR checks the given PR for changes to TEP markdown files. If one isn't present, it returns nil, +// and if one or more is present, their metadata is parsed and returned. +func (tgc *TEPGHClient) ExtractTEPInfoFromTEPPR(ctx context.Context, prNumber int, prRef string) ([]tep.TEPInfo, error) { + listOpts := &github.ListOptions{ + PerPage: 20, + } + + tepFilenamesToIDs := make(map[string]string) + + for { + files, resp, err := tgc.client.PullRequests.ListFiles(ctx, TEPsOwner, TEPsRepo, prNumber, listOpts) + if err != nil { + return nil, errors.Wrapf(err, "listing files in PR #%d in %s/%s", prNumber, TEPsOwner, TEPsRepo) + } + + for _, f := range files { + if f.Status != nil && f.Filename != nil && (*f.Status == "added" || *f.Status == "modified") { + m := tepPRFilenameRegex.FindStringSubmatch(*f.Filename) + if len(m) > 0 { + tepFilenamesToIDs[*f.Filename] = m[1] + } + } + } + if resp.NextPage == 0 { + break + } + listOpts.Page = resp.NextPage + } + + if len(tepFilenamesToIDs) == 0 { + return nil, fmt.Errorf("no TEP Markdown files found in PR #%d in %s/%s", prNumber, TEPsOwner, TEPsRepo) + } + + var teps []tep.TEPInfo + + for fn, tepID := range tepFilenamesToIDs { + fc, _, _, err := tgc.client.Repositories.GetContents(ctx, TEPsOwner, TEPsRepo, fn, &github.RepositoryContentGetOptions{ + Ref: prRef, + }) + if err != nil { + return nil, errors.Wrapf(err, "fetching contents of https://github.com/%s/%s/blob/%s/%s", TEPsOwner, TEPsRepo, + prRef, fn) + } + + mdStr, err := fc.GetContent() + if err != nil { + return nil, errors.Wrapf(err, "reading contents of https://github.com/%s/%s/blob/%s/%s", TEPsOwner, TEPsRepo, + prRef, fn) + } + + fnWithoutPrefix := strings.TrimPrefix(fn, "teps/") + info, err := tep.TEPInfoFromMarkdown(tepID, fnWithoutPrefix, mdStr) + if err != nil { + return nil, errors.Wrapf(err, "parsing contents of https://github.com/%s/%s/blob/%s/%s", TEPsOwner, TEPsRepo, + prRef, fn) + } + + teps = append(teps, info) + } + + return teps, nil +} + +// GetTrackingIssuesOptions allows configuring calls to GetTrackingIssues to filter on issue state, TEP status, or TEP ID +type GetTrackingIssuesOptions struct { + IssueState string + TEPStatus tep.Status + TEPID string +} + +// GetTrackingIssues returns issues in tektoncd/community with the "tep-tracking" label, optionally filtering by issue +// state, current known TEP status, or TEP ID. +func (tgc *TEPGHClient) GetTrackingIssues(ctx context.Context, opts *GetTrackingIssuesOptions) (map[string]tep.TrackingIssue, error) { + listOpts := &github.IssueListByRepoOptions{ + Labels: []string{TrackingIssueLabel}, + ListOptions: github.ListOptions{ + PerPage: 20, + }, + } + + tepID := "" + if opts != nil { + if opts.IssueState != "" { + listOpts.State = opts.IssueState + } + if opts.TEPStatus != "" { + listOpts.Labels = append(listOpts.Labels, opts.TEPStatus.TrackingLabel()) + } + tepID = opts.TEPID + } + + foundIssues := make(map[string]tep.TrackingIssue) + + for { + issues, resp, err := tgc.client.Issues.ListByRepo(ctx, TEPsOwner, TEPsRepo, listOpts) + if err != nil { + return nil, errors.Wrapf(err, "listing tracking issues in %s/%s", TEPsOwner, TEPsRepo) + } + + for _, iss := range issues { + if iss.IsPullRequest() { + continue + } + + if tepID != "" { + if iss.GetTitle() != fmt.Sprintf(TrackingIssueTitleFmt, tepID) { + continue + } + } + + ti := tep.TrackingIssue{ + IssueNumber: iss.GetNumber(), + IssueState: iss.GetState(), + Assignees: []string{}, + } + + if m := trackingIssueTitleRegex.FindStringSubmatch(iss.GetTitle()); len(m) > 0 { + ti.TEPID = m[1] + } + + for _, assignee := range iss.Assignees { + ti.Assignees = append(ti.Assignees, assignee.GetLogin()) + } + + for _, label := range iss.Labels { + if strings.HasPrefix(label.GetName(), tep.TrackingIssueStatusLabelPrefix) { + tepStatus := tep.FromTrackingIssueLabel(label.GetName()) + if tepStatus == nil { + return nil, fmt.Errorf("label %s is not a valid TEP status", label.GetName()) + } + ti.TEPStatus = *tepStatus + } + } + + tepPRIDs, implPRs, err := tep.PRsForTrackingIssue(iss.GetBody()) + if err != nil { + return nil, errors.Wrapf(err, "parsing TEP and implementation PRs from tracking issue body '%s'", iss.GetBody()) + } + + ti.TEPPRs = tepPRIDs + ti.ImplementationPRs = implPRs + + foundIssues[ti.TEPID] = ti + } + if resp.NextPage == 0 { + break + } + listOpts.Page = resp.NextPage + } + + return foundIssues, nil +} diff --git a/bots/tep-automation/pkg/ghclient/ghclient_test.go b/bots/tep-automation/pkg/ghclient/ghclient_test.go new file mode 100644 index 0000000000..a32f678ad0 --- /dev/null +++ b/bots/tep-automation/pkg/ghclient/ghclient_test.go @@ -0,0 +1,864 @@ +package ghclient_test + +import ( + "context" + "encoding/json" + "fmt" + "io/ioutil" + "net/http" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-github/v41/github" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/ghclient" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/testutil" +) + +func TestGetTEPsFromReadme(t *testing.T) { + testCases := []struct { + name string + respContent string + expectedTEPs map[string]tep.TEPInfo + }{ + { + name: "none", + respContent: testutil.GHContentJSON("nothing"), + expectedTEPs: make(map[string]tep.TEPInfo), + }, + { + name: "one TEP", + respContent: `there's one tep in here +on a later line +|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-20 | +|[TEP-5678](5678-not-valid-line.md) | | proposed | 2021-12-20 | +tada, a single valid TEP and a bogus line +`, + expectedTEPs: map[string]tep.TEPInfo{ + "1234": { + ID: "1234", + Title: "Some TEP Title", + Status: tep.ProposedStatus, + Filename: "1234-something-or-other.md", + LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), + }, + }, + }, + { + name: "three TEPs", + respContent: testutil.DefaultTEPReadmeContent, + expectedTEPs: map[string]tep.TEPInfo{ + "1234": { + ID: "1234", + Title: "Some TEP Title", + Status: tep.ProposedStatus, + Filename: "1234-something-or-other.md", + LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), + }, + "5678": { + ID: "5678", + Title: "Another TEP Title", + Status: tep.ProposedStatus, + Filename: "5678-second-one.md", + LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), + }, + "4321": { + ID: "4321", + Title: "Yet Another TEP Title", + Status: tep.ImplementingStatus, + Filename: "4321-third-one.md", + LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + client, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + mux.HandleFunc(testutil.ReadmeURL, + func(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.RequestURI, fmt.Sprintf("?ref=%s", ghclient.TEPsBranch)) { + t.Errorf("expected request for branch %s, but URI was %s", ghclient.TEPsBranch, r.RequestURI) + } + _, _ = fmt.Fprint(w, testutil.GHContentJSON(tc.respContent)) + }) + + tgc := ghclient.NewTEPGHClient(client) + + ctx := context.Background() + + teps, err := tgc.GetTEPsFromReadme(ctx) + require.NoError(t, err) + + if d := cmp.Diff(tc.expectedTEPs, teps); d != "" { + t.Errorf("Wrong TEPs from README.md: (-want, +got): %s", d) + } + }) + } +} + +type testComment struct { + id int64 + user string + body string +} + +func TestTEPCommentsOnPR(t *testing.T) { + testCases := []struct { + name string + comments []testComment + expected []tep.CommentInfo + }{ + { + name: "none with bot user comment", + comments: []testComment{{ + id: 1, + user: ghclient.BotUser, + body: "There are no TEPs here", + }}, + }, + { + name: "one with bot user comment", + comments: []testComment{{ + id: 1, + user: ghclient.BotUser, + body: `this comment has some text +and it also has a TEP + + + + +`, + }}, + expected: []tep.CommentInfo{{ + CommentID: 1, + TEPs: []string{"1234"}, + ToImplemented: false, + }}, + }, + { + name: "both implementing and implemented comments", + comments: []testComment{ + { + id: 1, + user: ghclient.BotUser, + body: `this comment has some text +and it also has a TEP + + + + +`, + }, + { + id: 2, + user: ghclient.BotUser, + body: `close this TEP + + + + +`, + }, + }, + expected: []tep.CommentInfo{ + { + CommentID: 1, + TEPs: []string{"1234"}, + ToImplemented: false, + }, + { + CommentID: 2, + TEPs: []string{"1234"}, + ToImplemented: true, + }, + }, + }, + { + name: "one with other user comment", + comments: []testComment{{ + id: 1, + user: "abayer", + body: `this comment has some text +and it also has a TEP + + +`, + }}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + client, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/issues/1/comments", ghclient.TEPsOwner, ghclient.TEPsRepo), + func(w http.ResponseWriter, r *http.Request) { + var ghComments []*github.IssueComment + + for _, c := range tc.comments { + cCopy := c + ghComments = append(ghComments, &github.IssueComment{ + ID: &cCopy.id, + User: &github.User{ + Login: &cCopy.user, + }, + Body: &cCopy.body, + }) + } + + respBody, err := json.Marshal(ghComments) + if err != nil { + t.Fatal("marshalling GitHub comments") + } + _, _ = fmt.Fprint(w, string(respBody)) + }) + + tgc := ghclient.NewTEPGHClient(client) + + ctx := context.Background() + + tepComments, err := tgc.TEPCommentsOnPR(ctx, ghclient.TEPsRepo, 1) + require.NoError(t, err) + + assert.ElementsMatch(t, tc.expected, tepComments) + }) + } +} + +func TestAddComment(t *testing.T) { + client, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + input := &github.IssueComment{ + Body: github.String("some body"), + } + + mux.HandleFunc("/repos/tektoncd/pipeline/issues/1/comments", + func(w http.ResponseWriter, r *http.Request) { + v := new(github.IssueComment) + require.NoError(t, json.NewDecoder(r.Body).Decode(v)) + + require.Equal(t, "POST", r.Method) + + if d := cmp.Diff(input, v); d != "" { + t.Errorf("difference in POST body: %s", d) + } + _, _ = fmt.Fprint(w, `{"id":1}`) + }) + + tgc := ghclient.NewTEPGHClient(client) + + ctx := context.Background() + + require.NoError(t, tgc.AddComment(ctx, "pipeline", 1, "some body")) +} + +func TestEditComment(t *testing.T) { + client, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + input := &github.IssueComment{ + Body: github.String("some new body"), + } + + mux.HandleFunc("/repos/tektoncd/pipeline/issues/comments/1", + func(w http.ResponseWriter, r *http.Request) { + v := new(github.IssueComment) + require.NoError(t, json.NewDecoder(r.Body).Decode(v)) + + require.Equal(t, "PATCH", r.Method) + + if d := cmp.Diff(input, v); d != "" { + t.Errorf("difference in PATCH body: %s", d) + } + _, _ = fmt.Fprint(w, `{"id":1}`) + }) + + tgc := ghclient.NewTEPGHClient(client) + + ctx := context.Background() + + require.NoError(t, tgc.EditComment(ctx, "pipeline", 1, "some new body")) +} + +func TestCreateTrackingIssue(t *testing.T) { + client, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + expectedTitle := "TEP-12345 Tracking Issue" + expectedBody := "some body" + expectedLabels := []string{ + ghclient.TrackingIssueLabel, + tep.NewStatus.TrackingLabel(), + } + expectedAssignees := []string{ + "abayer", + "vdemeester", + } + + input := &github.IssueRequest{ + Title: &expectedTitle, + Body: &expectedBody, + Labels: &expectedLabels, + Assignees: &expectedAssignees, + } + + mux.HandleFunc("/repos/tektoncd/community/issues", + func(w http.ResponseWriter, r *http.Request) { + v := new(github.IssueRequest) + require.NoError(t, json.NewDecoder(r.Body).Decode(v)) + + require.Equal(t, "POST", r.Method) + + if d := cmp.Diff(input, v); d != "" { + t.Errorf("difference in POST body: %s", d) + } + _, _ = fmt.Fprint(w, `{"id":1}`) + }) + + tgc := ghclient.NewTEPGHClient(client) + + ctx := context.Background() + + require.NoError(t, tgc.CreateTrackingIssue(ctx, "12345", expectedBody, []string{"abayer", "vdemeester"}, tep.NewStatus)) +} + +func TestUpdateTrackingIssue(t *testing.T) { + client, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + expectedTitle := "TEP-12345 Tracking Issue" + expectedBody := "some body" + expectedLabels := []string{ + ghclient.TrackingIssueLabel, + tep.NewStatus.TrackingLabel(), + } + expectedAssignees := []string{ + "abayer", + "vdemeester", + } + + input := &github.IssueRequest{ + Title: &expectedTitle, + Body: &expectedBody, + Labels: &expectedLabels, + Assignees: &expectedAssignees, + } + + issueNumber := 5 + + mux.HandleFunc(fmt.Sprintf("/repos/tektoncd/community/issues/%d", issueNumber), + func(w http.ResponseWriter, r *http.Request) { + v := new(github.IssueRequest) + require.NoError(t, json.NewDecoder(r.Body).Decode(v)) + + require.Equal(t, "PATCH", r.Method) + + if d := cmp.Diff(input, v); d != "" { + t.Errorf("difference in PATCH body: %s", d) + } + _, _ = fmt.Fprint(w, `{"number":5}`) + }) + + tgc := ghclient.NewTEPGHClient(client) + + ctx := context.Background() + + require.NoError(t, tgc.UpdateTrackingIssue(ctx, issueNumber, "12345", expectedBody, []string{"abayer", "vdemeester"}, tep.NewStatus)) +} + +func TestExtractTEPInfoFromTEPPR(t *testing.T) { + prNumber := 1 + contentsRef := "some-ref" + + testCases := []struct { + name string + listFilesResponse []*github.CommitFile + teps []tep.TEPInfo + expectedErr error + }{ + { + name: "no files", + listFilesResponse: []*github.CommitFile{}, + expectedErr: fmt.Errorf("no TEP Markdown files found in PR #%d in %s/%s", prNumber, ghclient.TEPsOwner, ghclient.TEPsRepo), + }, + { + name: "new TEP file", + listFilesResponse: []*github.CommitFile{{ + SHA: github.String("some-sha"), + Filename: github.String("teps/1234-some-proposal.md"), + Status: github.String("added"), + }}, + teps: []tep.TEPInfo{{ + ID: "1234", + Title: "Some New Feature", + Status: tep.ProposedStatus, + Filename: "1234-some-proposal.md", + LastModified: time.Date(2022, time.January, 6, 0, 0, 0, 0, time.UTC), + Authors: []string{ + "abayer", + "vdemeester", + }, + }}, + }, + { + name: "modified TEP file", + listFilesResponse: []*github.CommitFile{{ + SHA: github.String("some-sha"), + Filename: github.String("teps/1234-some-proposal.md"), + Status: github.String("modified"), + }}, + teps: []tep.TEPInfo{{ + ID: "1234", + Title: "Some New Feature", + Status: tep.ProposedStatus, + Filename: "1234-some-proposal.md", + LastModified: time.Date(2022, time.January, 6, 0, 0, 0, 0, time.UTC), + Authors: []string{ + "abayer", + "vdemeester", + }, + }}, + }, + { + name: "multiple TEP files", + listFilesResponse: []*github.CommitFile{ + { + SHA: github.String("some-sha"), + Filename: github.String("teps/1234-some-proposal.md"), + Status: github.String("added"), + }, + { + SHA: github.String("some-sha"), + Filename: github.String("teps/5678-just-cats.md"), + Status: github.String("modified"), + }, + }, + teps: []tep.TEPInfo{{ + ID: "5678", + Title: "Just Show Cats", + Status: tep.ImplementedStatus, + Filename: "5678-just-cats.md", + LastModified: time.Date(2021, time.January, 6, 0, 0, 0, 0, time.UTC), + Authors: []string{ + "abayer", + "bobcatfish", + }, + }, { + ID: "1234", + Title: "Some New Feature", + Status: tep.ProposedStatus, + Filename: "1234-some-proposal.md", + LastModified: time.Date(2022, time.January, 6, 0, 0, 0, 0, time.UTC), + Authors: []string{ + "abayer", + "vdemeester", + }, + }}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + client, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/pulls/%d/files", ghclient.TEPsOwner, ghclient.TEPsRepo, prNumber), + func(w http.ResponseWriter, r *http.Request) { + respBody, err := json.Marshal(tc.listFilesResponse) + if err != nil { + t.Fatal("marshalling GitHub file list") + } + _, _ = fmt.Fprint(w, string(respBody)) + }) + + for _, f := range tc.listFilesResponse { + fn := strings.TrimPrefix(*f.Filename, "teps/") + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/contents/teps/%s", ghclient.TEPsOwner, ghclient.TEPsRepo, fn), + func(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.RequestURI, fmt.Sprintf("?ref=%s", contentsRef)) { + t.Errorf("expected request for ref %s, but URI was %s", contentsRef, r.RequestURI) + } + + fileContent, err := ioutil.ReadFile(filepath.Join("testdata", fn)) + if err != nil { + t.Fatalf("reading testdata/%s", fn) + } + + _, _ = fmt.Fprint(w, testutil.GHContentJSON(string(fileContent))) + }) + } + tgc := ghclient.NewTEPGHClient(client) + + ctx := context.Background() + + foundTEPs, err := tgc.ExtractTEPInfoFromTEPPR(ctx, prNumber, contentsRef) + + if tc.expectedErr != nil { + require.EqualError(t, err, tc.expectedErr.Error()) + } else { + require.NoError(t, err) + + assert.ElementsMatch(t, tc.teps, foundTEPs, "wrong TEPs") + } + }) + } +} + +type testIssue struct { + number int + title string + body string + state string + labels []string + assignees []string +} + +func TestGetTrackingIssues(t *testing.T) { + testCases := []struct { + name string + allIssues []testIssue + listOpts *ghclient.GetTrackingIssuesOptions + expectedIssues map[string]tep.TrackingIssue + expectedErr error + }{ + { + name: "no issues", + expectedIssues: map[string]tep.TrackingIssue{}, + }, + { + name: "no tracking issues", + allIssues: []testIssue{{ + number: 5, + title: "some other issue", + body: "no TEP stuff in here", + state: "open", + labels: []string{"some-other-label"}, + assignees: []string{"bob"}, + }}, + expectedIssues: map[string]tep.TrackingIssue{}, + }, + { + name: "one tracking issue without PRs", + allIssues: []testIssue{{ + number: 7, + title: "TEP-1234 Tracking Issue", + body: "Nothing in here", + state: "open", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.ProposedStatus.TrackingLabel(), + }, + assignees: []string{"bob", "steve"}, + }}, + expectedIssues: map[string]tep.TrackingIssue{ + "1234": { + IssueNumber: 7, + IssueState: "open", + TEPStatus: tep.ProposedStatus, + TEPID: "1234", + Assignees: []string{"bob", "steve"}, + }, + }, + }, + { + name: "one tracking issue with PRs", + allIssues: []testIssue{{ + number: 7, + title: "TEP-1234 Tracking Issue", + body: `Something in here this time + + + +`, + state: "open", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.ProposedStatus.TrackingLabel(), + }, + assignees: []string{"bob", "steve"}, + }}, + expectedIssues: map[string]tep.TrackingIssue{ + "1234": { + IssueNumber: 7, + IssueState: "open", + TEPStatus: tep.ProposedStatus, + TEPID: "1234", + TEPPRs: []int{55, 66}, + ImplementationPRs: []tep.ImplementationPR{ + { + Repo: "pipeline", + Number: 77, + }, + { + Repo: "triggers", + Number: 88, + }, + }, + Assignees: []string{"bob", "steve"}, + }, + }, + }, + { + name: "filter by issue state", + allIssues: []testIssue{ + { + number: 6, + title: "TEP-0001 Tracking Issue", + body: "Nothing in here", + state: "closed", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.ImplementedStatus.TrackingLabel(), + }, + assignees: []string{"foo", "bar"}, + }, + { + number: 7, + title: "TEP-1234 Tracking Issue", + body: "Nothing in here", + state: "open", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.ProposedStatus.TrackingLabel(), + }, + assignees: []string{"bob", "steve"}, + }, + { + number: 9, + title: "TEP-0002 Tracking Issue", + body: "Nothing in here", + state: "closed", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.WithdrawnStatus.TrackingLabel(), + }, + assignees: []string{"a", "b"}, + }, + }, + listOpts: &ghclient.GetTrackingIssuesOptions{ + IssueState: "open", + }, + expectedIssues: map[string]tep.TrackingIssue{ + "1234": { + IssueNumber: 7, + IssueState: "open", + TEPStatus: tep.ProposedStatus, + TEPID: "1234", + Assignees: []string{"bob", "steve"}, + }, + }, + }, + { + name: "filter by TEP status", + allIssues: []testIssue{ + { + number: 6, + title: "TEP-0001 Tracking Issue", + body: "Nothing in here", + state: "closed", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.ImplementedStatus.TrackingLabel(), + }, + assignees: []string{"foo", "bar"}, + }, + { + number: 7, + title: "TEP-1234 Tracking Issue", + body: "Nothing in here", + state: "open", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.ProposedStatus.TrackingLabel(), + }, + assignees: []string{"bob", "steve"}, + }, + { + number: 9, + title: "TEP-0002 Tracking Issue", + body: "Nothing in here", + state: "closed", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.WithdrawnStatus.TrackingLabel(), + }, + assignees: []string{"a", "b"}, + }, + }, + listOpts: &ghclient.GetTrackingIssuesOptions{ + TEPStatus: tep.ProposedStatus, + }, + expectedIssues: map[string]tep.TrackingIssue{ + "1234": { + IssueNumber: 7, + IssueState: "open", + TEPStatus: tep.ProposedStatus, + TEPID: "1234", + Assignees: []string{"bob", "steve"}, + }, + }, + }, + { + name: "filter by TEP ID", + allIssues: []testIssue{ + { + number: 6, + title: "TEP-0001 Tracking Issue", + body: "Nothing in here", + state: "closed", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.ImplementedStatus.TrackingLabel(), + }, + assignees: []string{"foo", "bar"}, + }, + { + number: 7, + title: "TEP-1234 Tracking Issue", + body: "Nothing in here", + state: "open", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.ProposedStatus.TrackingLabel(), + }, + assignees: []string{"bob", "steve"}, + }, + { + number: 9, + title: "TEP-0002 Tracking Issue", + body: "Nothing in here", + state: "closed", + labels: []string{ + ghclient.TrackingIssueLabel, + tep.WithdrawnStatus.TrackingLabel(), + }, + assignees: []string{"a", "b"}, + }, + }, + listOpts: &ghclient.GetTrackingIssuesOptions{ + TEPID: "1234", + }, + expectedIssues: map[string]tep.TrackingIssue{ + "1234": { + IssueNumber: 7, + IssueState: "open", + TEPStatus: tep.ProposedStatus, + TEPID: "1234", + Assignees: []string{"bob", "steve"}, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + client, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/issues", ghclient.TEPsOwner, ghclient.TEPsRepo), + func(w http.ResponseWriter, r *http.Request) { + var issues []*github.Issue + + if err := r.ParseForm(); err != nil { + t.Fatalf("couldn't parse form: %s", err) + } + + filterState := "" + filterTEPStatus := "" + + if tc.listOpts != nil { + if tc.listOpts.IssueState != "" { + filterState = tc.listOpts.IssueState + assert.Equal(t, tc.listOpts.IssueState, r.Form.Get("state")) + } + if tc.listOpts.TEPStatus != "" { + filterTEPStatus = tc.listOpts.TEPStatus.TrackingLabel() + assert.Contains(t, r.Form.Get("labels"), tc.listOpts.TEPStatus.TrackingLabel()) + } + } + assert.Contains(t, r.Form.Get("labels"), ghclient.TrackingIssueLabel) + + for _, i := range tc.allIssues { + if filterState != "" && i.state != filterState { + continue + } + if filterTEPStatus != "" { + foundLabel := false + for _, l := range i.labels { + if l == filterTEPStatus { + foundLabel = true + break + } + } + if !foundLabel { + continue + } + } + + hasTrackingLabel := false + for _, l := range i.labels { + if l == ghclient.TrackingIssueLabel { + hasTrackingLabel = true + break + } + } + if !hasTrackingLabel { + continue + } + + ghIssue := github.Issue{ + Number: github.Int(i.number), + State: github.String(i.state), + Title: github.String(i.title), + Body: github.String(i.body), + Labels: []*github.Label{{ + Name: github.String(ghclient.TrackingIssueLabel), + }}, + Assignees: []*github.User{}, + } + + for _, l := range i.labels { + ghIssue.Labels = append(ghIssue.Labels, &github.Label{Name: github.String(l)}) + } + + for _, a := range i.assignees { + ghIssue.Assignees = append(ghIssue.Assignees, &github.User{Login: github.String(a)}) + } + + issues = append(issues, &ghIssue) + } + + respBody, err := json.Marshal(issues) + if err != nil { + t.Fatal("marshalling GitHub issues") + } + _, _ = fmt.Fprint(w, string(respBody)) + }) + + tgc := ghclient.NewTEPGHClient(client) + + ctx := context.Background() + + trackingIssues, err := tgc.GetTrackingIssues(ctx, tc.listOpts) + + if tc.expectedErr != nil { + require.EqualError(t, err, tc.expectedErr.Error()) + } else { + require.NoError(t, err) + + assert.Equal(t, tc.expectedIssues, trackingIssues, "wrong tracking issues") + } + }) + } +} diff --git a/bots/tep-automation/pkg/ghclient/testdata/1234-some-proposal.md b/bots/tep-automation/pkg/ghclient/testdata/1234-some-proposal.md new file mode 100644 index 0000000000..5fea8b3d4e --- /dev/null +++ b/bots/tep-automation/pkg/ghclient/testdata/1234-some-proposal.md @@ -0,0 +1,12 @@ +--- +title: Some New Feature +authors: +- "@abayer" +- "@vdemeester" +creation-date: 2021-12-10 +last-updated: 2022-01-06 +status: proposed +--- + +SOME OTHER CONTENT + diff --git a/bots/tep-automation/pkg/ghclient/testdata/5678-just-cats.md b/bots/tep-automation/pkg/ghclient/testdata/5678-just-cats.md new file mode 100644 index 0000000000..53e35468de --- /dev/null +++ b/bots/tep-automation/pkg/ghclient/testdata/5678-just-cats.md @@ -0,0 +1,11 @@ +--- +title: Just Show Cats +authors: +- "@abayer" +- "@bobcatfish" +creation-date: 2020-12-10 +last-updated: 2021-01-06 +status: implemented +--- + +SOME OTHER CONTENT diff --git a/bots/tep-automation/pkg/performers/issuecreator.go b/bots/tep-automation/pkg/performers/issuecreator.go new file mode 100644 index 0000000000..21d40dce01 --- /dev/null +++ b/bots/tep-automation/pkg/performers/issuecreator.go @@ -0,0 +1,88 @@ +package performers + +import ( + "context" + + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/ghclient" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + "knative.dev/pkg/logging" + kreconciler "knative.dev/pkg/reconciler" +) + +// IssueCreator is a Performer that responds to newly opened PRs to the community repository by creating or updating +// tracking issues, if appropriate. +type IssueCreator struct { + GHClient *ghclient.TEPGHClient +} + +// NewIssueCreator creates a new IssueCreator instance configured with a GitHub client +func NewIssueCreator(ghClient *ghclient.TEPGHClient) *IssueCreator { + return &IssueCreator{ + GHClient: ghClient, + } +} + +// NewIssueCreatorAsPerformer calls NewIssueCreator and returns it as a Performer +func NewIssueCreatorAsPerformer(ghClient *ghclient.TEPGHClient) Performer { + return NewIssueCreator(ghClient) +} + +// Perform checks if the action for the PR is opened or edited and if it contains a TEP markdown file. If so, it creates +// (or updates) a tracking issue for it. +func (p *IssueCreator) Perform(ctx context.Context, opts *PerformerOptions) kreconciler.Event { + // Skip out if the repo is not community + if opts.Repo != "community" { + return nil + } + + logger := logging.FromContext(ctx).With(zap.String("performer", "IssueCreator")) + logger.Infof("Performing tracking issue creation/updating (if needed) for %s/%s", opts.RunNamespace, opts.RunName) + + // Short-circuit for PR actions other than `edited` or `opened`. + if opts.Action != closedAction && opts.Action != editedAction { + logger.Infof("Ignoring PR action %s; will do nothing", opts.Action) + return nil + } + + tepsInPR, err := p.GHClient.ExtractTEPInfoFromTEPPR(ctx, opts.PRNumber, opts.GitRevision) + if err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "LoadingPRTEPs", "Failure finding TEP markdown changes for %s/%s PR #%d: %s", + ghclient.TEPsOwner, opts.Repo, opts.PRNumber, err) + } + + trackingIssues, err := p.GHClient.GetTrackingIssues(ctx, &ghclient.GetTrackingIssuesOptions{IssueState: "open"}) + if err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "LoadingTrackingIssues", "Failure loading existing tracking issues: %s", err) + } + + // TODO(abayer): actually do stuff + for _, t := range tepsInPR { + if issue, ok := trackingIssues[t.ID]; ok { + issueBody, err := issue.GetBody(t.Filename) + if err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "TrackingIssueBody", "Failure generating tracking issue body for issue %d: %s", issue.IssueNumber, err) + } + if err := p.GHClient.UpdateTrackingIssue(ctx, issue.IssueNumber, t.ID, issueBody, t.Authors, t.Status); err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "UpdatingTrackingIssue", "Failure updating tracking issue for issue %d: %s", issue.IssueNumber, err) + } + } else { + issue := tep.TrackingIssue{ + TEPStatus: t.Status, + TEPID: t.ID, + TEPPRs: []int{opts.PRNumber}, + Assignees: t.Authors, + } + issueBody, err := issue.GetBody(t.Filename) + if err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "TrackingIssueBody", "Failure generating tracking issue body for TEP-%s: %s", t.ID, err) + } + if err := p.GHClient.CreateTrackingIssue(ctx, t.ID, issueBody, t.Authors, t.Status); err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "CreatingTrackingIssue", "Failure updating tracking issue for TEP-%s: %s", t.ID, err) + } + } + } + + return nil +} diff --git a/bots/tep-automation/pkg/performers/prnotifier.go b/bots/tep-automation/pkg/performers/prnotifier.go new file mode 100644 index 0000000000..fc3e90b064 --- /dev/null +++ b/bots/tep-automation/pkg/performers/prnotifier.go @@ -0,0 +1,216 @@ +package performers + +import ( + "context" + "fmt" + "strings" + + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/ghclient" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + "knative.dev/pkg/logging" + kreconciler "knative.dev/pkg/reconciler" +) + +const ( + // TEPCommentAndStatusRegexFmt is the format used for adding the metadata for TEP and status to the comment. + TEPCommentAndStatusRegexFmt = "\n" + + // ToImplementedCommentHeader is the header for PR comments reminding to update a given TEP from the + // `implementing` status to the `implemented` status. + ToImplementedCommentHeader = "This merged pull request appears to be referencing one or more [Tekton Enhancement Proposals](https://github.com/tektoncd/community/tree/main/teps#readme) " + + "which are currently in the `implementing` state. If this PR finished the work towards implementing these TEPs, " + + "please update their state(s) to `implemented`.\n" + + "\n" + + "TEPs:\n" + // ToImplementingCommentHeader is the header for PR comments reminding to update a given TEP from the + // `proposed` or `implementable` status to the `implementing` status. + ToImplementingCommentHeader = "This pull request appears to be referencing one or more [Tekton Enhancement Proposals](https://github.com/tektoncd/community/tree/main/teps#readme) " + + "which are currently in the `proposed` or `implementable` states. If this PR contains " + + "work towards implementing these TEPs, please update their state(s) to `implementing`.\n" + + "\n" + + "TEPs:\n" + + closedAction = "closed" + openedAction = "opened" + editedAction = "edited" +) + +// PRNotifier handles the actual parsing of PR title and description for TEP identifiers, and adds comments to the PR +// as appropriate. +type PRNotifier struct { + GHClient *ghclient.TEPGHClient +} + +// NewPRNotifier creates a new PRNotifier instance configured with a GitHub client +func NewPRNotifier(ghClient *ghclient.TEPGHClient) *PRNotifier { + return &PRNotifier{ + GHClient: ghClient, + } +} + +// NewPRNotifierAsPerformer calls NewPRNotifier and returns it as a Performer +func NewPRNotifierAsPerformer(ghClient *ghclient.TEPGHClient) Performer { + return NewPRNotifier(ghClient) +} + +// Perform checks if the PR needs to have a TEP-related commented added or updated, and if so, does so. +func (n *PRNotifier) Perform(ctx context.Context, opts *PerformerOptions) kreconciler.Event { + logger := logging.FromContext(ctx).With(zap.String("performer", "PRNotifier")) + logger.Infof("Performing implementation PR notification (if needed) for %s/%s", opts.RunNamespace, opts.RunName) + + // Short-circuit for PR actions other than `closed`, `edited`, or `opened`. + if opts.Action != closedAction && opts.Action != editedAction && opts.Action != openedAction { + logger.Infof("Ignoring PR action %s; will do nothing", opts.Action) + return nil + } + + // Short-circuit if the action is "closed" but the PR is not merged. + if opts.Action == closedAction && !opts.IsMerged { + logger.Info("Ignoring closed PR because the PR was not merged; will do nothing") + return nil + } + + tepsOnPR, err := n.TEPsInPR(ctx, opts.Title, opts.Body) + if err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "LoadingPRTEPs", "Failure loading TEPs for %s/%s PR #%d: %s", ghclient.TEPsOwner, opts.Repo, opts.PRNumber, err) + } + + var tepsForComment []tep.TEPInfo + var commentFunc func([]tep.TEPInfo) string + var transitionStates []tep.Status + + if opts.Action == closedAction { + commentFunc = PRMergedComment + transitionStates = append(transitionStates, tep.ImplementingStatus) + } else { + commentFunc = PROpenedComment + transitionStates = append(transitionStates, tep.ProposedStatus, tep.ImplementableStatus) + } + + for _, t := range tepsOnPR { + shouldInclude := false + for _, ts := range transitionStates { + if ts == t.Status { + shouldInclude = true + } + } + + if shouldInclude { + tepsForComment = append(tepsForComment, t) + } + } + + if len(tepsForComment) > 0 { + commentBody := commentFunc(tepsForComment) + + existingComments, err := n.GHClient.TEPCommentsOnPR(ctx, opts.Repo, opts.PRNumber) + if err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "CheckingPRComments", "Failure checking for TEP comments for %s/%s PR #%d: %s", + ghclient.TEPsOwner, opts.Repo, opts.PRNumber, err) + } + + var commentToUpdate *tep.CommentInfo + for _, cmt := range existingComments { + if opts.Action == closedAction && cmt.ToImplemented { + commentToUpdate = &cmt + break + } else if opts.Action == editedAction || opts.Action == openedAction { + commentToUpdate = &cmt + } + } + + if commentToUpdate != nil { + needsUpdate := false + for _, newTep := range tepsForComment { + found := false + for _, id := range commentToUpdate.TEPs { + if id == newTep.ID { + found = true + break + } + } + if !found { + needsUpdate = true + break + } + } + + if needsUpdate { + err = n.GHClient.EditComment(ctx, opts.Repo, commentToUpdate.CommentID, commentBody) + if err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "UpdatingPRComment", "Failure updating existing comment %d for %s/%s PR #%d: %s", + commentToUpdate.CommentID, ghclient.TEPsOwner, opts.Repo, opts.PRNumber, err) + } + return kreconciler.NewEvent(corev1.EventTypeNormal, "CommentUpdated", "Existing comment %d for %s/%s PR #%d updated", + commentToUpdate.CommentID, ghclient.TEPsOwner, opts.Repo, opts.PRNumber) + } + } else { + err = n.GHClient.AddComment(ctx, opts.Repo, opts.PRNumber, commentBody) + if err != nil { + return kreconciler.NewEvent(corev1.EventTypeWarning, "AddingPRComment", "Failure adding new comment for %s/%s PR #%d: %s", + ghclient.TEPsOwner, opts.Repo, opts.PRNumber, err) + } + return kreconciler.NewEvent(corev1.EventTypeNormal, "CommentAdded", "Comment for %s/%s PR #%d", + ghclient.TEPsOwner, opts.Repo, opts.PRNumber) + } + } + + // If we got here, then we didn't need to do anything. + logger.Infof("No TEPs found in title or body for %s/%s PR #%d; nothing to do", ghclient.TEPsOwner, opts.Repo, opts.PRNumber) + return nil +} + +// TEPsInPR returns all TEPs referenced in the PR title or body in a map with the TEP ID as key and information about the +// TEP as value. +func (n *PRNotifier) TEPsInPR(ctx context.Context, prTitle, prBody string) (map[string]tep.TEPInfo, error) { + tepsWithInfo := make(map[string]tep.TEPInfo) + + tepIDs := tep.GetTEPIDsFromPR(prTitle, prBody) + + if len(tepIDs) == 0 { + return tepsWithInfo, nil + } + + allTEPs, err := n.GHClient.GetTEPsFromReadme(ctx) + if err != nil { + return nil, err + } + + for _, tID := range tepIDs { + tepsWithInfo[tID] = allTEPs[tID] + } + + return tepsWithInfo, nil +} + +// PROpenedComment returns the appropriate GitHub Markdown-formatted content for the PR comment on opened/edited PRs +// referencing TEPs in `proposed` or `implementable` states. +func PROpenedComment(teps []tep.TEPInfo) string { + return generatePRComment(ToImplementingCommentHeader, teps) +} + +// PRMergedComment returns the appropriate GitHub Markdown-formatted content for the PR comment on merged PRs +// referencing TEPs in the `implementing` state. +func PRMergedComment(teps []tep.TEPInfo) string { + return generatePRComment(ToImplementedCommentHeader, teps) +} + +func generatePRComment(header string, teps []tep.TEPInfo) string { + var commentLines []string + var listLines []string + var metadataLines []string + + for _, t := range teps { + listLines = append(listLines, fmt.Sprintf(" * [TEP-%s (%s)](%s%s), current status: `%s`\n", t.ID, t.Title, tep.CommunityBlobBaseURL, t.Filename, t.Status)) + metadataLines = append(metadataLines, fmt.Sprintf(TEPCommentAndStatusRegexFmt, t.ID, t.Status)) + } + + commentLines = append(commentLines, header) + commentLines = append(commentLines, listLines...) + commentLines = append(commentLines, "\n") // Newline between the list of TEPs and the metadata + commentLines = append(commentLines, metadataLines...) + + return strings.Join(commentLines, "") // TODO(abayer): Probably should get rid of trailing newlines on the header and the list/metadata lines and just join on "\n" here. +} diff --git a/bots/tep-automation/pkg/performers/prnotifier_test.go b/bots/tep-automation/pkg/performers/prnotifier_test.go new file mode 100644 index 0000000000..3d63c23980 --- /dev/null +++ b/bots/tep-automation/pkg/performers/prnotifier_test.go @@ -0,0 +1,297 @@ +package performers_test + +import ( + "context" + "encoding/json" + "fmt" + "io/ioutil" + "net/http" + "testing" + "time" + + "github.com/google/go-github/v41/github" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/ghclient" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/performers" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/testutil" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kreconciler "knative.dev/pkg/reconciler" +) + +func TestPROpenedComment(t *testing.T) { + teps := []tep.TEPInfo{ + { + ID: "1234", + Title: "Some TEP Title", + Status: tep.ProposedStatus, + Filename: "1234-something-or-other.md", + LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), + }, + { + ID: "5678", + Title: "Some Other TEP Title", + Status: tep.ImplementableStatus, + Filename: "5678-insert-filename-here.md", + LastModified: time.Date(2021, time.December, 21, 0, 0, 0, 0, time.UTC), + }, + } + + expectedComment := performers.ToImplementingCommentHeader + + " * [TEP-1234 (Some TEP Title)](https://github.com/tektoncd/community/blob/main/teps/1234-something-or-other.md), current status: `proposed`\n" + + " * [TEP-5678 (Some Other TEP Title)](https://github.com/tektoncd/community/blob/main/teps/5678-insert-filename-here.md), current status: `implementable`\n" + + "\n" + + "\n" + + "\n" + + assert.Equal(t, expectedComment, performers.PROpenedComment(teps)) +} + +func TestPRClosedComment(t *testing.T) { + teps := []tep.TEPInfo{ + { + ID: "1234", + Title: "Some TEP Title", + Status: tep.ImplementingStatus, + Filename: "1234-something-or-other.md", + LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), + }, + { + ID: "5678", + Title: "Some Other TEP Title", + Status: tep.ImplementingStatus, + Filename: "5678-insert-filename-here.md", + LastModified: time.Date(2021, time.December, 21, 0, 0, 0, 0, time.UTC), + }, + } + + expectedComment := performers.ToImplementedCommentHeader + + " * [TEP-1234 (Some TEP Title)](https://github.com/tektoncd/community/blob/main/teps/1234-something-or-other.md), current status: `implementing`\n" + + " * [TEP-5678 (Some Other TEP Title)](https://github.com/tektoncd/community/blob/main/teps/5678-insert-filename-here.md), current status: `implementing`\n" + + "\n" + + "\n" + + "\n" + + assert.Equal(t, expectedComment, performers.PRMergedComment(teps)) +} + +func TestPerform(t *testing.T) { + testCases := []struct { + name string + paramOverrides map[string]string + additionalParams map[string]string + requests map[string]func(w http.ResponseWriter, r *http.Request) + doesNothing bool + expectedEventType string + expectedReason string + expectedErr error + }{ + { + name: "no TEPs", + doesNothing: true, + }, + { + name: "wrong action", + paramOverrides: map[string]string{ + tep.ActionParamName: "assigned", + }, + doesNothing: true, + }, + { + name: "closed but unmerged", + paramOverrides: map[string]string{ + tep.ActionParamName: "closed", + }, + doesNothing: true, + }, + { + name: "fetching README 404", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-1234", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "LoadingPRTEPs", + }, + { + name: "fetching PR comments 404", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-1234", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "CheckingPRComments", + }, + { + name: "adding comment for opened PR", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-1234", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": testutil.NoCommentsOnPRHandlerFunc(t), + }, + expectedEventType: corev1.EventTypeNormal, + expectedReason: "CommentAdded", + }, + { + name: "editing comment for opened PR", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-1234", + tep.PRBodyParamName: "With a body referencing TEP-5678", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "GET", r.Method) + + commentID := int64(1) + commentUser := ghclient.BotUser + commentBody := fmt.Sprintf("%s* [TEP-1234] (Some TEP Title)][https://github.com/tektoncd/community/blob/main/teps/1234-something-or-other.md),"+ + "current status: `proposed`\n\n\n", performers.ToImplementingCommentHeader) + comments := []*github.IssueComment{{ + ID: &commentID, + Body: &commentBody, + User: &github.User{ + Login: &commentUser, + }, + }} + respBody, err := json.Marshal(comments) + if err != nil { + t.Fatal("marshalling GitHub comments") + } + _, _ = fmt.Fprint(w, string(respBody)) + }, + "/repos/tektoncd/pipeline/issues/comments/1": func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "PATCH", r.Method) + body, err := ioutil.ReadAll(r.Body) + require.NoError(t, err) + require.Contains(t, string(body), "TEP-5678") + _, _ = fmt.Fprint(w, `{"id":1}`) + }, + }, + expectedEventType: corev1.EventTypeNormal, + expectedReason: "CommentUpdated", + }, + { + name: "wrong state for opened PR", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-4321", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": testutil.NoCommentsOnPRHandlerFunc(t), + }, + doesNothing: true, + }, + { + name: "wrong state for closed PR", + paramOverrides: map[string]string{ + tep.ActionParamName: "closed", + tep.PRTitleParamName: "PR referencing TEP-1234", + tep.PRIsMergedParamName: "true", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": testutil.NoCommentsOnPRHandlerFunc(t), + }, + doesNothing: true, + }, + { + name: "adding comment for closed PR", + paramOverrides: map[string]string{ + tep.ActionParamName: "closed", + tep.PRTitleParamName: "PR referencing TEP-4321", + tep.PRIsMergedParamName: "true", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case "GET": + commentID := int64(1) + commentUser := ghclient.BotUser + commentBody := fmt.Sprintf("%s* [TEP-4321] (Some TEP Title)][https://github.com/tektoncd/community/blob/main/teps/4321-something-or-other.md),"+ + "current status: `implementable`\n\n\n", performers.ToImplementingCommentHeader) + comments := []*github.IssueComment{{ + ID: &commentID, + Body: &commentBody, + User: &github.User{ + Login: &commentUser, + }, + }} + respBody, err := json.Marshal(comments) + if err != nil { + t.Fatal("marshalling GitHub comments") + } + _, _ = fmt.Fprint(w, string(respBody)) + return + case "POST": + _, _ = fmt.Fprint(w, `{"id":1}`) + return + default: + t.Errorf("unexpected method %s", r.Method) + } + }, + }, + expectedEventType: corev1.EventTypeNormal, + expectedReason: "CommentAdded", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + + ghClient, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + tgc := ghclient.NewTEPGHClient(ghClient) + + for k, v := range tc.requests { + mux.HandleFunc(k, v) + } + + n := performers.NewPRNotifier(tgc) + + run := &v1alpha1.Run{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-reconcile-run", + Namespace: "foo", + }, + Spec: v1alpha1.RunSpec{ + Params: testutil.ConstructRunParams(tc.paramOverrides, tc.additionalParams), + }, + } + + opts, err := performers.ToPerformerOptions(run) + require.NoError(t, err) + + err = n.Perform(ctx, opts) + if tc.expectedErr != nil { + assert.Equal(t, tc.expectedErr, err) + } else { + if tc.doesNothing { + require.Nil(t, err) + } else { + require.NotNil(t, err) + recEvt, ok := err.(*kreconciler.ReconcilerEvent) + if !ok { + t.Fatalf("did not expect non-ReconcilerEvent error %s", recEvt) + } else { + if recEvt.EventType != tc.expectedEventType { + t.Errorf("Expected event type to be %s but was %s", tc.expectedEventType, recEvt.EventType) + } + if recEvt.Reason != tc.expectedReason { + t.Errorf("Expected reason to be %q but was %q", tc.expectedReason, recEvt.Reason) + } + } + } + } + }) + } +} diff --git a/bots/tep-automation/pkg/performers/types.go b/bots/tep-automation/pkg/performers/types.go new file mode 100644 index 0000000000..bc8f1e3f4e --- /dev/null +++ b/bots/tep-automation/pkg/performers/types.go @@ -0,0 +1,114 @@ +package performers + +import ( + "context" + "strconv" + "strings" + + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" + corev1 "k8s.io/api/core/v1" + + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + kreconciler "knative.dev/pkg/reconciler" +) + +// Performer is an interface to actually perform the action for a given event +type Performer interface { + Perform(ctx context.Context, opts *PerformerOptions) kreconciler.Event +} + +// PerformerOptions contains the parameters passed to the Run, processed into an appropriate form for Performers to use. +type PerformerOptions struct { + RunName string + RunNamespace string + Action string + PRNumber int + Title string + Body string + Repo string + IsMerged bool + GitRevision string +} + +// ToPerformerOptions extracts known parameters from a run's spec, and returns a configured PerformerOptions, or an error +// if a parameter is missing or an invalid value, or has too many parameters. +func ToPerformerOptions(run *v1alpha1.Run) (*PerformerOptions, error) { + if len(run.Spec.Params) > 7 { + var found []string + for _, p := range run.Spec.Params { + if p.Name == tep.ActionParamName || + p.Name == tep.PRNumberParamName || + p.Name == tep.PRTitleParamName || + p.Name == tep.PRBodyParamName || + p.Name == tep.PackageParamName || + p.Name == tep.PRIsMergedParamName || + p.Name == tep.GitRevisionParamName { + continue + } + found = append(found, p.Name) + } + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "UnexpectedParams", "Found unexpected params: %v", found) + } + + opts := &PerformerOptions{ + RunName: run.Name, + RunNamespace: run.Namespace, + } + + prActionParam := run.Spec.GetParam(tep.ActionParamName) + if prActionParam == nil || prActionParam.Value.StringVal == "" { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "MissingPullRequestAction", "The %s param was not passed", tep.ActionParamName) + } + opts.Action = prActionParam.Value.StringVal + + prNumberParam := run.Spec.GetParam(tep.PRNumberParamName) + if prNumberParam == nil || prNumberParam.Value.StringVal == "" { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "MissingPullRequestNumber", "The %s param was not passed", tep.PRNumberParamName) + } + prNumber, err := strconv.Atoi(prNumberParam.Value.StringVal) + if err != nil { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "InvalidPullRequestNumber", "%s is not a valid value for the %s param", prNumberParam.Value.StringVal, tep.PRNumberParamName) + } + opts.PRNumber = prNumber + + prTitleParam := run.Spec.GetParam(tep.PRTitleParamName) + if prTitleParam == nil || prTitleParam.Value.StringVal == "" { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "MissingPullRequestTitle", "The %s param was not passed", tep.PRTitleParamName) + } + opts.Title = prTitleParam.Value.StringVal + + prBodyParam := run.Spec.GetParam(tep.PRBodyParamName) + if prBodyParam == nil || prBodyParam.Value.StringVal == "" { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "MissingPullRequestBody", "The %s param was not passed", tep.PRBodyParamName) + } + opts.Body = prBodyParam.Value.StringVal + + orgAndRepoParam := run.Spec.GetParam(tep.PackageParamName) + if orgAndRepoParam == nil || orgAndRepoParam.Value.StringVal == "" { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "MissingPackage", "The %s param was not passed", tep.PackageParamName) + } + splitOrgAndRepo := strings.Split(orgAndRepoParam.Value.StringVal, "/") + if len(splitOrgAndRepo) < 2 { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "InvalidPackage", "The %s param value %s does not contain an owner and a repository seperated by '/'", + tep.PackageParamName, orgAndRepoParam.Value.StringVal) + } + opts.Repo = splitOrgAndRepo[1] + + isMergedParam := run.Spec.GetParam(tep.PRIsMergedParamName) + if isMergedParam == nil || isMergedParam.Value.StringVal == "" { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "MissingPullRequestIsMerged", "The %s param was not passed", tep.PRIsMergedParamName) + } + isMerged, err := strconv.ParseBool(isMergedParam.Value.StringVal) + if err != nil { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "InvalidPullRequestIsMerged", "%s is not a valid value for the %s param", isMergedParam.Value.StringVal, tep.PRIsMergedParamName) + } + opts.IsMerged = isMerged + + gitRevParam := run.Spec.GetParam(tep.GitRevisionParamName) + if gitRevParam == nil || gitRevParam.Value.StringVal == "" { + return nil, kreconciler.NewEvent(corev1.EventTypeWarning, "MissingPullRequestSHA", "The %s param was not passed", tep.GitRevisionParamName) + } + opts.GitRevision = gitRevParam.Value.StringVal + + return opts, nil +} diff --git a/bots/tep-automation/pkg/performers/types_test.go b/bots/tep-automation/pkg/performers/types_test.go new file mode 100644 index 0000000000..2f21522449 --- /dev/null +++ b/bots/tep-automation/pkg/performers/types_test.go @@ -0,0 +1,132 @@ +package performers_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/performers" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/testutil" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kreconciler "knative.dev/pkg/reconciler" +) + +func TestToPerformerOptions(t *testing.T) { + testCases := []struct { + name string + paramOverrides map[string]string + additionalParams map[string]string + expectedEventType string + expectedReason string + }{ + { + name: "all params valid", + }, + { + name: "missing action", + paramOverrides: map[string]string{ + tep.ActionParamName: "", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "MissingPullRequestAction", + }, + { + name: "missing PR number", + paramOverrides: map[string]string{ + tep.PRNumberParamName: "", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "MissingPullRequestNumber", + }, + { + name: "missing PR title", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "MissingPullRequestTitle", + }, + { + name: "missing PR body", + paramOverrides: map[string]string{ + tep.PRBodyParamName: "", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "MissingPullRequestBody", + }, + { + name: "missing package", + paramOverrides: map[string]string{ + tep.PackageParamName: "", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "MissingPackage", + }, + { + name: "missing PR isMerged", + paramOverrides: map[string]string{ + tep.PRIsMergedParamName: "", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "MissingPullRequestIsMerged", + }, + { + name: "invalid PR number", + paramOverrides: map[string]string{ + tep.PRNumberParamName: "banana", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "InvalidPullRequestNumber", + }, + { + name: "invalid package", + paramOverrides: map[string]string{ + tep.PackageParamName: "not-owner-slash-repo", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "InvalidPackage", + }, + { + name: "invalid additional param", + additionalParams: map[string]string{ + "something": "or other", + }, + expectedEventType: corev1.EventTypeWarning, + expectedReason: "UnexpectedParams", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + run := &v1alpha1.Run{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-reconcile-run", + Namespace: "foo", + }, + Spec: v1alpha1.RunSpec{ + Params: testutil.ConstructRunParams(tc.paramOverrides, tc.additionalParams), + }, + } + + _, err := performers.ToPerformerOptions(run) + if tc.expectedReason != "" || tc.expectedEventType != "" { + require.NotNil(t, err) + recEvt, ok := err.(*kreconciler.ReconcilerEvent) + if !ok { + t.Fatalf("did not expect non-ReconcilerEvent error %s", recEvt) + } else { + if recEvt.EventType != tc.expectedEventType { + t.Errorf("Expected event type to be %s but was %s", tc.expectedEventType, recEvt.EventType) + } + if recEvt.Reason != tc.expectedReason { + t.Errorf("Expected reason to be %q but was %q", tc.expectedReason, recEvt.Reason) + } + } + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/tep-notifier/pkg/reconciler/controller.go b/bots/tep-automation/pkg/reconciler/controller.go similarity index 80% rename from tep-notifier/pkg/reconciler/controller.go rename to bots/tep-automation/pkg/reconciler/controller.go index 48a15b718e..e79a0ce923 100644 --- a/tep-notifier/pkg/reconciler/controller.go +++ b/bots/tep-automation/pkg/reconciler/controller.go @@ -7,6 +7,8 @@ import ( runinformer "github.com/tektoncd/pipeline/pkg/client/injection/informers/pipeline/v1alpha1/run" runreconciler "github.com/tektoncd/pipeline/pkg/client/injection/reconciler/pipeline/v1alpha1/run" tkncontroller "github.com/tektoncd/pipeline/pkg/controller" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/ghclient" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/performers" "k8s.io/client-go/tools/cache" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" @@ -21,12 +23,12 @@ const ( ) // NewController instantiates a new controller.Impl from knative.dev/pkg/controller -func NewController(ghToken string) func(context.Context, configmap.Watcher) *controller.Impl { +func NewController(ghToken string, performerFuncs ...func(*ghclient.TEPGHClient) performers.Performer) func(context.Context, configmap.Watcher) *controller.Impl { return func(ctx context.Context, cmw configmap.Watcher) *controller.Impl { logger := logging.FromContext(ctx) runInformer := runinformer.Get(ctx) - r := NewReconciler(ctx, ghToken) + r := NewReconciler(ctx, ghToken, performerFuncs...) impl := runreconciler.NewImpl(ctx, r, func(impl *controller.Impl) controller.Options { return controller.Options{ diff --git a/bots/tep-automation/pkg/reconciler/reconciler.go b/bots/tep-automation/pkg/reconciler/reconciler.go new file mode 100644 index 0000000000..d48a1de158 --- /dev/null +++ b/bots/tep-automation/pkg/reconciler/reconciler.go @@ -0,0 +1,118 @@ +package reconciler + +import ( + "context" + "sync" + + "github.com/hashicorp/go-multierror" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/ghclient" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/performers" + corev1 "k8s.io/api/core/v1" + "knative.dev/pkg/logging" + kreconciler "knative.dev/pkg/reconciler" +) + +// Reconciler handles the actual parsing of PR title and description for TEP identifiers, and adds comments to the PR +// as appropriate. +type Reconciler struct { + GHClient *ghclient.TEPGHClient + Performers []performers.Performer +} + +// NewReconciler creates a new Reconciler instance configured with a GitHub client +func NewReconciler(ctx context.Context, ghToken string, performerFuncs ...func(*ghclient.TEPGHClient) performers.Performer) *Reconciler { + tgc := ghclient.NewTEPGHClientFromToken(ctx, ghToken) + r := &Reconciler{ + GHClient: tgc, + Performers: []performers.Performer{}, + } + + for _, pf := range performerFuncs { + r.Performers = append(r.Performers, pf(tgc)) + } + + return r +} + +// ReconcileKind implements Interface.ReconcileKind. +func (r *Reconciler) ReconcileKind(ctx context.Context, run *v1alpha1.Run) kreconciler.Event { + logger := logging.FromContext(ctx) + logger.Infof("Reconciling %s/%s", run.Namespace, run.Name) + + // Ignore completed waits. + if run.IsDone() { + logger.Info("Run is finished, done reconciling") + return nil + } + + if run.Spec.Ref == nil || + run.Spec.Ref.APIVersion != v1beta1.SchemeGroupVersion.String() || run.Spec.Ref.Kind != Kind { + logger.Warnf("Should not have been notified about Run %s/%s; will do nothing", run.Namespace, run.Name) + return nil + } + + performerOpts, err := performers.ToPerformerOptions(run) + if err != nil { + if recEvt, ok := err.(*kreconciler.ReconcilerEvent); ok { + run.Status.MarkRunFailed(recEvt.Reason, recEvt.Format, recEvt.Args) + } else { + run.Status.MarkRunFailed("UnknownError", "unknown error occurred: %s", err) + } + return nil + } + + var results []error + mu := &sync.Mutex{} + var wg sync.WaitGroup + wg.Add(len(r.Performers)) + + for _, performer := range r.Performers { + go func(p performers.Performer) { + defer wg.Done() + res := p.Perform(ctx, performerOpts) + mu.Lock() + results = append(results, res) + mu.Unlock() + }(performer) + } + + wg.Wait() + + hasResult := false + var failures []error + for _, res := range results { + if res != nil { + hasResult = true + recEvt, ok := res.(*kreconciler.ReconcilerEvent) + if !ok { + failures = append(failures, res) + } else if recEvt.EventType != corev1.EventTypeNormal { + failures = append(failures, res) + } + } + } + + // If none of the performers returned a non-nil event, don't do anything to the run status + if !hasResult { + return nil + } + + switch len(failures) { + case 0: + run.Status.MarkRunSucceeded("AllSucceeded", "TEP Automation successful") + case 1: + if recEvt, ok := failures[0].(*kreconciler.ReconcilerEvent); ok { + run.Status.MarkRunFailed(recEvt.Reason, recEvt.Format, recEvt.Args) + } else { + run.Status.MarkRunFailed("UnknownError", "unknown error occurred: %s", failures[0]) + } + default: + var allErrors error + allErrors = multierror.Append(allErrors, failures...) + run.Status.MarkRunFailed("MultipleErrors", "multiple errors: %s", allErrors) + } + + return nil +} diff --git a/bots/tep-automation/pkg/reconciler/reconciler_test.go b/bots/tep-automation/pkg/reconciler/reconciler_test.go new file mode 100644 index 0000000000..2327b3e343 --- /dev/null +++ b/bots/tep-automation/pkg/reconciler/reconciler_test.go @@ -0,0 +1,331 @@ +package reconciler_test + +import ( + "context" + "encoding/json" + "fmt" + "io/ioutil" + "net/http" + "testing" + + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" + + "github.com/google/go-github/v41/github" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/ghclient" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/performers" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/reconciler" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/testutil" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/apis" +) + +func TestReconcileKind(t *testing.T) { + defaultKindRef := &v1beta1.TaskRef{ + APIVersion: v1beta1.SchemeGroupVersion.String(), + Kind: reconciler.Kind, + } + + testCases := []struct { + name string + paramOverrides map[string]string + additionalParams map[string]string + kindRef *v1beta1.TaskRef + requests map[string]func(w http.ResponseWriter, r *http.Request) + doesNothing bool + expectedStatus corev1.ConditionStatus + expectedReason string + expectedErr error + }{ + { + name: "no TEPs", + doesNothing: true, + }, + { + name: "invalid ref", + kindRef: &v1beta1.TaskRef{ + APIVersion: v1beta1.SchemeGroupVersion.String(), + Kind: "SomethingElse", + }, + doesNothing: true, + }, + { + name: "wrong action", + paramOverrides: map[string]string{ + tep.ActionParamName: "assigned", + }, + doesNothing: true, + }, + { + name: "closed but unmerged", + paramOverrides: map[string]string{ + tep.ActionParamName: "closed", + }, + doesNothing: true, + }, + { + name: "missing action", + paramOverrides: map[string]string{ + tep.ActionParamName: "", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "MissingPullRequestAction", + }, + { + name: "missing PR number", + paramOverrides: map[string]string{ + tep.PRNumberParamName: "", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "MissingPullRequestNumber", + }, + { + name: "missing PR title", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "MissingPullRequestTitle", + }, + { + name: "missing PR body", + paramOverrides: map[string]string{ + tep.PRBodyParamName: "", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "MissingPullRequestBody", + }, + { + name: "missing package", + paramOverrides: map[string]string{ + tep.PackageParamName: "", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "MissingPackage", + }, + { + name: "missing PR isMerged", + paramOverrides: map[string]string{ + tep.PRIsMergedParamName: "", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "MissingPullRequestIsMerged", + }, + { + name: "invalid PR number", + paramOverrides: map[string]string{ + tep.PRNumberParamName: "banana", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "InvalidPullRequestNumber", + }, + { + name: "invalid package", + paramOverrides: map[string]string{ + tep.PackageParamName: "not-owner-slash-repo", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "InvalidPackage", + }, + { + name: "invalid additional param", + additionalParams: map[string]string{ + "something": "or other", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "UnexpectedParams", + }, + { + name: "fetching README 404", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-1234", + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "LoadingPRTEPs", + }, + { + name: "fetching PR comments 404", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-1234", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + }, + expectedStatus: corev1.ConditionFalse, + expectedReason: "CheckingPRComments", + }, + { + name: "adding comment for opened PR", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-1234", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": testutil.NoCommentsOnPRHandlerFunc(t), + }, + expectedStatus: corev1.ConditionTrue, + expectedReason: "AllSucceeded", + }, + { + name: "editing comment for opened PR", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-1234", + tep.PRBodyParamName: "With a body referencing TEP-5678", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "GET", r.Method) + + commentID := int64(1) + commentUser := ghclient.BotUser + commentBody := fmt.Sprintf("%s* [TEP-1234] (Some TEP Title)][https://github.com/tektoncd/community/blob/main/teps/1234-something-or-other.md),"+ + "current status: `proposed`\n\n\n", performers.ToImplementingCommentHeader) + comments := []*github.IssueComment{{ + ID: &commentID, + Body: &commentBody, + User: &github.User{ + Login: &commentUser, + }, + }} + respBody, err := json.Marshal(comments) + if err != nil { + t.Fatal("marshalling GitHub comments") + } + _, _ = fmt.Fprint(w, string(respBody)) + }, + "/repos/tektoncd/pipeline/issues/comments/1": func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "PATCH", r.Method) + body, err := ioutil.ReadAll(r.Body) + require.NoError(t, err) + require.Contains(t, string(body), "TEP-5678") + _, _ = fmt.Fprint(w, `{"id":1}`) + }, + }, + expectedStatus: corev1.ConditionTrue, + expectedReason: "AllSucceeded", + }, + { + name: "wrong state for opened PR", + paramOverrides: map[string]string{ + tep.PRTitleParamName: "PR referencing TEP-4321", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": testutil.NoCommentsOnPRHandlerFunc(t), + }, + doesNothing: true, + }, + { + name: "wrong state for closed PR", + paramOverrides: map[string]string{ + tep.ActionParamName: "closed", + tep.PRTitleParamName: "PR referencing TEP-1234", + tep.PRIsMergedParamName: "true", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": testutil.NoCommentsOnPRHandlerFunc(t), + }, + doesNothing: true, + }, + { + name: "adding comment for closed PR", + paramOverrides: map[string]string{ + tep.ActionParamName: "closed", + tep.PRTitleParamName: "PR referencing TEP-4321", + tep.PRIsMergedParamName: "true", + }, + requests: map[string]func(w http.ResponseWriter, r *http.Request){ + testutil.ReadmeURL: testutil.DefaultREADMEHandlerFunc(), + "/repos/tektoncd/pipeline/issues/1/comments": func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case "GET": + commentID := int64(1) + commentUser := ghclient.BotUser + commentBody := fmt.Sprintf("%s* [TEP-4321] (Some TEP Title)][https://github.com/tektoncd/community/blob/main/teps/4321-something-or-other.md),"+ + "current status: `implementable`\n\n\n", performers.ToImplementingCommentHeader) + comments := []*github.IssueComment{{ + ID: &commentID, + Body: &commentBody, + User: &github.User{ + Login: &commentUser, + }, + }} + respBody, err := json.Marshal(comments) + if err != nil { + t.Fatal("marshalling GitHub comments") + } + _, _ = fmt.Fprint(w, string(respBody)) + return + case "POST": + _, _ = fmt.Fprint(w, `{"id":1}`) + return + default: + t.Errorf("unexpected method %s", r.Method) + } + }, + }, + expectedStatus: corev1.ConditionTrue, + expectedReason: "AllSucceeded", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + + ghClient, mux, closeFunc := testutil.SetupFakeGitHub() + defer closeFunc() + + tgc := ghclient.NewTEPGHClient(ghClient) + for k, v := range tc.requests { + mux.HandleFunc(k, v) + } + + r := &reconciler.Reconciler{ + GHClient: tgc, + Performers: []performers.Performer{performers.NewPRNotifier(tgc)}, + } + + run := &v1alpha1.Run{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-reconcile-run", + Namespace: "foo", + }, + Spec: v1alpha1.RunSpec{ + Params: testutil.ConstructRunParams(tc.paramOverrides, tc.additionalParams), + }, + } + if tc.kindRef != nil { + run.Spec.Ref = tc.kindRef + } else { + run.Spec.Ref = defaultKindRef + } + + err := r.ReconcileKind(ctx, run) + if tc.expectedErr != nil { + assert.Equal(t, tc.expectedErr, err) + } else { + require.NoError(t, err) + condition := run.Status.GetCondition(apis.ConditionSucceeded) + if tc.doesNothing { + assert.Nil(t, condition) + } else { + require.NotNil(t, condition, "Condition missing in Run") + + if condition.Status != tc.expectedStatus { + t.Errorf("Expected Run status to be %v but was %v", tc.expectedStatus, condition) + } + if condition.Reason != tc.expectedReason { + t.Errorf("Expected reason to be %q but was %q", tc.expectedReason, condition.Reason) + } + } + } + }) + } +} diff --git a/bots/tep-automation/pkg/tep/parser.go b/bots/tep-automation/pkg/tep/parser.go new file mode 100644 index 0000000000..effee591f0 --- /dev/null +++ b/bots/tep-automation/pkg/tep/parser.go @@ -0,0 +1,208 @@ +package tep + +import ( + "bytes" + "fmt" + "regexp" + "strconv" + "strings" + "time" + + "github.com/pkg/errors" + "github.com/yuin/goldmark" + meta "github.com/yuin/goldmark-meta" + "github.com/yuin/goldmark/parser" +) + +var ( + // TEPsInReadme matches rows in the table in https://github.com/tektoncd/community/blob/main/teps/README.md + TEPsInReadme = regexp.MustCompile(`\|\[TEP-(\d+)]\((.*?\.md)\) \| (.*?) \| (.*?) \| (\d\d\d\d-\d\d-\d\d) \|`) + // NotifierActionRegex is used to detect whether a comment is referring to transitioning to implementing or to implemented. + NotifierActionRegex = regexp.MustCompile(``) + // IDRegex is used to parse out "TEP-1234" from PR bodies and titles. + IDRegex = regexp.MustCompile(`TEP-(\d+)`) + // URLRegex is used to parse out TEP URLs, like https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md, from + // PR bodies. We ignore the branch when looking for matches. + URLRegex = regexp.MustCompile(`https://github\.com/tektoncd/community/blob/.*?/teps/(\d+)-.*?\.md`) + // CommentAndStatusRegex is used to detect whether a comment already is being used for the reminder message for + // a particular TEP and status. + CommentAndStatusRegex = regexp.MustCompile(``) + // TrackingIssueTEPPRsRegex parses out references to community repo TEP PRs from an issue's body. + TrackingIssueTEPPRsRegex = regexp.MustCompile(``) + // TrackingIssueImplementationPRsRegex parses out references to PRs implementing a TEP from an issue's body. + TrackingIssueImplementationPRsRegex = regexp.MustCompile(``) +) + +// ExtractTEPsFromReadme takes the body of https://github.com/tektoncd/community/blob/main/teps/README.md and extracts a +// map of all TEP IDs (i.e., "1234" for TEP-1234) and their statuses. +func ExtractTEPsFromReadme(readmeBody string) (map[string]TEPInfo, error) { + teps := make(map[string]TEPInfo) + + for _, m := range TEPsInReadme.FindAllStringSubmatch(readmeBody, -1) { + if len(m) > 5 { + // TODO(abayer): For some reason, I can't ever get time.Parse to handle a format of "2021-12-20" so let's just pad it and do it as RFC3339. + lastMod, err := time.Parse(time.RFC3339, fmt.Sprintf("%sT00:00:00Z", m[5])) + if err != nil { + return nil, err + } + + if !IsValidStatus(m[4]) { + return nil, fmt.Errorf("%s is not a valid status", m[4]) + } + teps[m[1]] = TEPInfo{ + ID: m[1], + Title: m[3], + Status: Status(m[4]), + Filename: m[2], + LastModified: lastMod, + } + } + } + + return teps, nil +} + +// GetTEPCommentDetails looks at a PR comment and extracts any TEP IDs and their status in the comment and whether this +// comment is for transitioning TEPs to `implementing` or to `implemented` +func GetTEPCommentDetails(comment string) (map[string]string, bool) { + teps := make(map[string]string) + + toImplemented := false + notifierMatch := NotifierActionRegex.FindStringSubmatch(comment) + if len(notifierMatch) > 1 && notifierMatch[1] == string(ImplementedStatus) { + toImplemented = true + } + + for _, m := range CommentAndStatusRegex.FindAllStringSubmatch(comment, -1) { + if len(m) > 2 { + teps[m[1]] = m[2] + } + } + + return teps, toImplemented +} + +// GetTEPIDsFromPR extracts all TEP IDs and URLs in the given PR title and body +func GetTEPIDsFromPR(prTitle, prBody string) []string { + var tepIDs []string + + // Find "TEP-1234" in PR title + for _, m := range IDRegex.FindAllStringSubmatch(prTitle, -1) { + if len(m) > 1 { + tepIDs = append(tepIDs, m[1]) + } + } + // Find TEP URLs in title + for _, m := range URLRegex.FindAllStringSubmatch(prTitle, -1) { + if len(m) > 1 { + tepIDs = append(tepIDs, m[1]) + } + } + + // Find "TEP-1234" in PR body + for _, m := range IDRegex.FindAllStringSubmatch(prBody, -1) { + if len(m) > 1 { + tepIDs = append(tepIDs, m[1]) + } + } + // Find TEP URLs in body + for _, m := range URLRegex.FindAllStringSubmatch(prBody, -1) { + if len(m) > 1 { + tepIDs = append(tepIDs, m[1]) + } + } + + return tepIDs +} + +// TEPInfoFromMarkdown parses a TEP markdown file to get its ID, title, status, last modified time, and authors +func TEPInfoFromMarkdown(id string, filename string, contents string) (TEPInfo, error) { + info := TEPInfo{ + ID: id, + Filename: filename, + Authors: []string{}, + } + + markdown := goldmark.New( + goldmark.WithExtensions( + meta.Meta, + ), + ) + var buf bytes.Buffer + parserCtx := parser.NewContext() + if err := markdown.Convert([]byte(contents), &buf, parser.WithContext(parserCtx)); err != nil { + return info, err + } + md := meta.Get(parserCtx) + + title, ok := md["title"].(string) + if !ok { + return info, fmt.Errorf("metadata for title '%s' is not a string", md["title"]) + } + info.Title = title + + rawStatus, ok := md["status"].(string) + if !ok { + return info, fmt.Errorf("metadata for status '%s' is not a string", md["status"]) + } + if !IsValidStatus(rawStatus) { + return info, fmt.Errorf("metadata for status '%s' is not a valid status", rawStatus) + } + info.Status = Status(rawStatus) + + rawLastMod, ok := md["last-updated"].(string) + if !ok { + return info, fmt.Errorf("metadata for last-updated '%s' is not a string", md["last-updated"]) + } + lastMod, err := time.Parse(time.RFC3339, fmt.Sprintf("%sT00:00:00Z", rawLastMod)) + if err != nil { + return info, errors.Wrapf(err, "couldn't parse last-updated '%s'", rawLastMod) + } + info.LastModified = lastMod + + rawAuthors, ok := md["authors"].([]interface{}) + if !ok { + return info, fmt.Errorf("metadata for authors '%s' is not a slice", md["authors"]) + } + for _, ra := range rawAuthors { + a, ok := ra.(string) + if !ok { + return info, fmt.Errorf("metadata for individual author '%s' is not a string", ra) + } + info.Authors = append(info.Authors, strings.TrimPrefix(a, "@")) + } + + return info, nil +} + +// PRsForTrackingIssue parses the body of a tracking issue to find all TEP PRs and implementation PRs within the metadata +// in the body, and returns them. +func PRsForTrackingIssue(body string) ([]int, []ImplementationPR, error) { + var tepPRIDs []int + var implPRs []ImplementationPR + + for _, m := range TrackingIssueTEPPRsRegex.FindAllStringSubmatch(body, -1) { + if len(m) > 1 { + id, err := strconv.Atoi(m[1]) + if err != nil { + return nil, nil, err + } + tepPRIDs = append(tepPRIDs, id) + } + } + + for _, m := range TrackingIssueImplementationPRsRegex.FindAllStringSubmatch(body, -1) { + if len(m) > 1 { + id, err := strconv.Atoi(m[2]) + if err != nil { + return nil, nil, err + } + implPRs = append(implPRs, ImplementationPR{ + Repo: m[1], + Number: id, + }) + } + } + + return tepPRIDs, implPRs, nil +} diff --git a/bots/tep-automation/pkg/tep/parser_test.go b/bots/tep-automation/pkg/tep/parser_test.go new file mode 100644 index 0000000000..0eb45a72a4 --- /dev/null +++ b/bots/tep-automation/pkg/tep/parser_test.go @@ -0,0 +1,320 @@ +package tep_test + +import ( + "io/ioutil" + "path/filepath" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" +) + +func TestExtractTEPsFromReadme(t *testing.T) { + testCases := []struct { + name string + body string + expected map[string]tep.TEPInfo + errStr string + }{ + { + name: "no TEPs", + body: "there are no teps here", + expected: make(map[string]tep.TEPInfo), + }, + { + name: "single TEP", + body: `there's one tep in here +on a later line +|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-20 | +|[TEP-5678](5678-not-valid-line.md) | | proposed | 2021-12-20 | +tada, a single valid TEP and a bogus line +`, + expected: map[string]tep.TEPInfo{ + "1234": { + ID: "1234", + Title: "Some TEP Title", + Status: tep.ProposedStatus, + Filename: "1234-something-or-other.md", + LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), + }, + }, + }, + { + name: "multiple TEPs", + body: `there are two teps in here +on later lines +|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-20 | +|[TEP-5678](5678-valid-line-this-time.md) | A Second TEP Title | implemented | 2021-12-29 | +tada, a single valid TEP and a bogus line +`, + expected: map[string]tep.TEPInfo{ + "1234": { + ID: "1234", + Title: "Some TEP Title", + Status: tep.ProposedStatus, + Filename: "1234-something-or-other.md", + LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), + }, + "5678": { + ID: "5678", + Title: "A Second TEP Title", + Status: tep.ImplementedStatus, + Filename: "5678-valid-line-this-time.md", + LastModified: time.Date(2021, time.December, 29, 0, 0, 0, 0, time.UTC), + }, + }, + }, + { + name: "invalid date", + body: "|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-40 |", + errStr: `parsing time "2021-12-40T00:00:00Z": day out of range`, + }, + { + name: "invalid status", + body: "|[TEP-1234](1234-something-or-other.md) | Some TEP Title | invalid-status | 2021-12-20 |", + errStr: "invalid-status is not a valid status", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + teps, err := tep.ExtractTEPsFromReadme(tc.body) + if tc.errStr != "" { + require.EqualError(t, err, tc.errStr) + } else { + require.NoError(t, err) + } + assert.Equal(t, tc.expected, teps) + }) + } +} + +func TestGetTEPIDsFromPR(t *testing.T) { + testCases := []struct { + name string + title string + body string + ids []string + }{ + { + name: "none", + title: "Some PR title", + body: "Some PR body", + }, + { + name: "one id in title", + title: "This implements TEP-1234 perhaps", + body: "Some PR body", + ids: []string{"1234"}, + }, + { + name: "one id in body", + title: "Some PR title", + body: "This implements TEP-1234 perhaps", + ids: []string{"1234"}, + }, + { + name: "one url in title", + title: "This is for https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md", + body: "Some PR body", + ids: []string{"0002"}, + }, + { + name: "one url in body", + title: "Some PR title", + body: "This is for https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md", + ids: []string{"0002"}, + }, + { + name: "one id in title, one url in body", + title: "This is for TEP-1234", + body: "This is for https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md", + ids: []string{"0002", "1234"}, + }, + { + name: "two urls in body", + title: "Some PR title", + body: "This is for https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md and https://github.com/tektoncd/community/blob/main/teps/0006-tekton-metrics.md", + ids: []string{"0002", "0006"}, + }, + { + name: "two ids in body", + title: "Some PR title", + body: "This implements TEP-1234 perhaps. And what the heck, TEP-5678 as well.", + ids: []string{"1234", "5678"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + found := tep.GetTEPIDsFromPR(tc.title, tc.body) + assert.ElementsMatch(t, tc.ids, found) + }) + } +} + +func TestGetTEPCommentDetails(t *testing.T) { + testCases := []struct { + name string + comment string + teps map[string]string + toImplemented bool + }{ + { + name: "none", + comment: "this has no TEPs", + teps: make(map[string]string), + }, + { + name: "one TEP", + comment: `this comment has some text +and it also has a TEP + + + + +`, + toImplemented: true, + teps: map[string]string{ + "1234": "implementing", + }, + }, + { + name: "two TEPs", + comment: `this comment has some text +and it also has two TEPs + + + + + +`, + teps: map[string]string{ + "1234": string(tep.ProposedStatus), + "5678": string(tep.ImplementableStatus), + }, + toImplemented: false, + }, + { + name: "duplicate TEP", + comment: `this comment has some text +and it also has one TEP, duplicated with a different status + + + + + +`, + teps: map[string]string{ + "1234": string(tep.ImplementableStatus), + }, + toImplemented: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + teps, toImplemented := tep.GetTEPCommentDetails(tc.comment) + if d := cmp.Diff(tc.teps, teps); d != "" { + t.Errorf("Wrong TEPs from comment: (-want, +got): %s", d) + } + assert.Equalf(t, tc.toImplemented, toImplemented, "expected toImplemented to be %t, but is %t", tc.toImplemented, toImplemented) + }) + } +} + +func TestTEPInfoFromMarkdown(t *testing.T) { + mdFile := filepath.Join("testdata", "markdown", "0014-step-timeout.md") + mdContent, err := ioutil.ReadFile(mdFile) + require.NoError(t, err) + + info, err := tep.TEPInfoFromMarkdown("0014", "0014-step-timeout.md", string(mdContent)) + require.NoError(t, err) + + assert.Equal(t, "0014", info.ID) + assert.Equal(t, "0014-step-timeout.md", info.Filename) + assert.Equal(t, "Step Timeout", info.Title) + assert.Equal(t, time.Date(2021, time.December, 13, 0, 0, 0, 0, time.UTC), info.LastModified) + assert.Equal(t, tep.ImplementedStatus, info.Status) + assert.ElementsMatch(t, []string{"Peaorl"}, info.Authors) +} + +func TestPRsForTrackingIssue(t *testing.T) { + testCases := []struct { + name string + body string + tepPRs []int + implPRs []tep.ImplementationPR + expectedErr error + }{ + { + name: "none", + body: "nothing to see here", + }, + { + name: "one of each", + body: ` +`, + tepPRs: []int{55}, + implPRs: []tep.ImplementationPR{{ + Repo: "pipeline", + Number: 77, + }}, + }, + { + name: "one TEP PR", + body: "", + tepPRs: []int{55}, + }, + { + name: "one implementation PR", + body: "", + implPRs: []tep.ImplementationPR{{ + Repo: "pipeline", + Number: 77, + }}, + }, + { + name: "multiple of each", + body: ` + + +`, + tepPRs: []int{55, 66}, + implPRs: []tep.ImplementationPR{ + { + Repo: "pipeline", + Number: 77, + }, + { + Repo: "triggers", + Number: 88, + }, + }, + }, + { + name: "not a number for TEP PR", + body: "", + }, + { + name: "not a number for implementation PR", + body: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tepPRs, implPRs, err := tep.PRsForTrackingIssue(tc.body) + if tc.expectedErr != nil { + require.Equal(t, tc.expectedErr, err) + } else { + require.NoError(t, err) + assert.ElementsMatch(t, tc.tepPRs, tepPRs) + assert.Equal(t, tc.implPRs, implPRs) + } + }) + } +} diff --git a/bots/tep-automation/pkg/tep/status.go b/bots/tep-automation/pkg/tep/status.go new file mode 100644 index 0000000000..0145ac0dc7 --- /dev/null +++ b/bots/tep-automation/pkg/tep/status.go @@ -0,0 +1,88 @@ +package tep + +import ( + "fmt" + "strings" +) + +const ( + // NewStatus is used for unmerged TEPs. + NewStatus Status = "new" + // ProposedStatus is the "proposed" TEP status. + ProposedStatus Status = "proposed" + // ImplementableStatus is the "implementable" status. + ImplementableStatus Status = "implementable" + // ImplementingStatus is the "implementing" status. + ImplementingStatus Status = "implementing" + // ImplementedStatus is the "implemented" status. + ImplementedStatus Status = "implemented" + // WithdrawnStatus is the "withdrawn" status. + WithdrawnStatus Status = "withdrawn" + // ReplacedStatus is the "replaced" status. + ReplacedStatus Status = "replaced" + + // TrackingIssueStatusLabelPrefix is the prefix added to the Status to generate the label on GitHub for tracking issues. + TrackingIssueStatusLabelPrefix = "tep-status/" +) + +var ( + // Statuses is a list of all valid TEP statuses + Statuses = []Status{ + NewStatus, + ProposedStatus, + ImplementableStatus, + ImplementingStatus, + ImplementedStatus, + WithdrawnStatus, + ReplacedStatus, + } +) + +// Status is a valid TEP status +type Status string + +// TrackingLabel returns "tep-status/[status string]" for a status. +func (s Status) TrackingLabel() string { + return fmt.Sprintf("%s%s", TrackingIssueStatusLabelPrefix, s) +} + +// ForMarkdown returns the status surrounded by backticks for use in GitHub comments, issue bodies, etc. +func (s Status) ForMarkdown() string { + return fmt.Sprintf("`%s`", s) +} + +// FromTrackingIssueLabel extracts the status from the "tep-status/whatever" label and returns the appropriate Status. +// If the result isn't a valid status, it returns nil. +func FromTrackingIssueLabel(label string) *Status { + withoutPrefix := strings.TrimPrefix(label, TrackingIssueStatusLabelPrefix) + if IsValidStatus(withoutPrefix) { + asStatus := Status(withoutPrefix) + return &asStatus + } + + return nil +} + +// IsValidStatus returns true if the given string is a known status type +func IsValidStatus(s string) bool { + for _, status := range Statuses { + if s == string(status) { + return true + } + } + + return false +} + +// GetTEPsWithStatus filters a map of TEP ID to TEPInfo for all TEPs in a given status +func GetTEPsWithStatus(input map[string]TEPInfo, desiredStatus Status) map[string]TEPInfo { + teps := make(map[string]TEPInfo) + + for k, v := range input { + if v.Status == desiredStatus { + teps[k] = v + } + } + + return teps +} diff --git a/bots/tep-automation/pkg/tep/status_test.go b/bots/tep-automation/pkg/tep/status_test.go new file mode 100644 index 0000000000..90f8396d8f --- /dev/null +++ b/bots/tep-automation/pkg/tep/status_test.go @@ -0,0 +1,99 @@ +package tep_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" +) + +func TestGetTEPsWithStatus(t *testing.T) { + testCases := []struct { + name string + status tep.Status + input map[string]tep.TEPInfo + expectedIDs []string + }{ + { + name: "none", + status: tep.ProposedStatus, + input: map[string]tep.TEPInfo{ + "1234": { + ID: "1234", + Title: "Some TEP", + Status: tep.ImplementableStatus, + Filename: "1234.md", + LastModified: time.Time{}, + }, + }, + }, + { + name: "one match", + status: tep.ProposedStatus, + input: map[string]tep.TEPInfo{ + "1234": { + ID: "1234", + Title: "Some TEP", + Status: tep.ImplementableStatus, + Filename: "1234.md", + LastModified: time.Time{}, + }, + "4321": { + ID: "4321", + Title: "Some Other TEP", + Status: tep.ProposedStatus, + Filename: "4321.md", + LastModified: time.Time{}, + }, + }, + expectedIDs: []string{"4321"}, + }, + { + name: "two match", + status: tep.ProposedStatus, + input: map[string]tep.TEPInfo{ + "1234": { + ID: "1234", + Title: "Some TEP", + Status: tep.ImplementableStatus, + Filename: "1234.md", + LastModified: time.Time{}, + }, + "4321": { + ID: "4321", + Title: "Some Other TEP", + Status: tep.ProposedStatus, + Filename: "4321.md", + LastModified: time.Time{}, + }, + "5678": { + ID: "5678", + Title: "A Third TEP", + Status: tep.ImplementedStatus, + Filename: "5678.md", + LastModified: time.Time{}, + }, + "8765": { + ID: "8765", + Title: "A Fourth TEP", + Status: tep.ProposedStatus, + Filename: "8765.md", + LastModified: time.Time{}, + }, + }, + expectedIDs: []string{"4321", "8765"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + found := tep.GetTEPsWithStatus(tc.input, tc.status) + var foundIDs []string + for k := range found { + foundIDs = append(foundIDs, k) + } + assert.ElementsMatch(t, tc.expectedIDs, foundIDs) + }) + } +} diff --git a/bots/tep-automation/pkg/tep/testdata/markdown/0014-step-timeout.md b/bots/tep-automation/pkg/tep/testdata/markdown/0014-step-timeout.md new file mode 100644 index 0000000000..623e1d28b0 --- /dev/null +++ b/bots/tep-automation/pkg/tep/testdata/markdown/0014-step-timeout.md @@ -0,0 +1,172 @@ +--- +title: Step Timeout +authors: + - "@Peaorl" +creation-date: 2020-09-10 +last-updated: 2021-12-13 +status: implemented +--- + +# TEP-0014: Step timeout + + +- [Summary](#summary) +- [Motivation](#motivation) + - [Goals](#goals) + - [Non-Goals](#non-goals) +- [Requirements](#requirements) +- [Proposal](#proposal) + - [Risks and Mitigations](#risks-and-mitigations) +- [Design Details](#design-details) + - [Caveats](#caveats) + - [Resolution of a Step Timeout](#resolution-of-a--timeout) +- [Test Plan](#test-plan) +- [References](#references) + + +## Summary + +A `Step` could end up executing longer than expected. Currently, Tekton does +not provide a way of terminating an overdue `Step`. Therefore, this TEP proposes +a `Step` timeout feature. + +Implementing this TEP, every `Step` can be annotated with a `timeout` field. +If during runtime the `Step` execution time exceeds this timeout, the `Step` +is terminated. Moreover, any subsequently scheduled `Steps` within the `Task` +are canceled. + +In case a `Step` timeout occurred, the `TaskRun` status field displays an +accompanying error message. + + +## Motivation +A `Task` author may want to specify a timeout for numerous reason. +A few example use cases are listed below: + +- A `Task` author may expect a `Step` to only take a short period of time. For +example, a `Task` author may expect a `Step` responsible for performing setup +to only require a few seconds. If for some reason the `Step` execution time +is much longer, it may be favorable to *fail fast*. As such, a supposedly +trivial `Step` can't stall or delay a `TaskRun`. As a result, a `Task` author +is able to troubleshoot sooner. Furthermore, potentially costly cluster +resources are released quicker. + +- A dependency-fetching `Step` may hang because an external registry is +slowed down. In this case it may be better to *fail fast* and retry instead +of waiting for the connection to time out. + +- A team has reduced the compilation time of their codebase and would like to +ensure that new changes do not increase the compilation time substantially. +They enforce this by setting a timeout on the compilation `Step` in their build +`Task` and run this `Task` against all new PRs. + +Direct motivation for this TEP stems from +[this](https://github.com/tektoncd/pipeline/issues/1690) user story. + +### Goals + +- Provide the ability to terminate an overdue `Step` +- Cancel `Steps` originally scheduled after a timeout terminated `Step` + +### Non-Goals + +- Provide the ability to terminate an overdue `Sidecar` + +## Requirements + +1. Possibility to have a `Step` terminated after exceeding a `Task` author +specified timeout +1. Tekton should provide a reasonable timeout resolution of about 1 second at +most +1. `Steps` scheduled after a timeout terminated `Step` shall be canceled + +## Proposal + +`Task` authors will be able to annotate a `Step` with a `timeout` field as +displayed in the following example: +```yaml +steps: + - name: sleep-then-timeout + image: ubuntu + script: | + #!/usr/bin/env bash + echo "I am supposed to sleep for 60 seconds!" + sleep 60 + timeout: 5s +``` +In this example, the `Step` prints a message and intends to *sleep* for 60 seconds. +However, since a five second timeout is specified, Tekton terminates the `Step` after five seconds. + +Subsequently, Tekton populates the `status.conditions.message` field in the initiating +`TaskRun` with the following message: + +`sleep-then-timeout exited because the step exceeded the specified timeout limit;` + +Additionally, if successive `Steps` were specified, Tekton cancels all these +successive `Steps` and indicate this with *exit code* 1 under +`status.steps.terminated.exitCode` of the `TaskRun`. + +### Risks and Mitigations + +The duration of a timeout is entirely up to the `Task` author. It is therefore +the `Task` author's responsibility to ensure a timeout provides a `Step` +enough time to properly execute. Performance variability amongst clusters may +require a suitable margin on a timeout. + +## Design Details + +The root of the design is centered around the preexisting Tekton entrypoint +binary. This binary overrides the original entrypoint of the *container* +associated with a `Step`. The Tekton entrypoint binary executes the command +or script specified by a `Step`. + +The design presented here essentially wires a timeout annotation from a +`Step` through to the Tekton entrypoint binary. The Tekton entrypoint binary +is modified to ensure it adheres to the specified timeout. Therefore, a +`Step` is automatically terminated once the timeout is exceeded. +Subsequently, Tekton writes a +*PostFile* indicating the `Step` has been terminated, thereby cancelling any +successive `Steps`. + +In order to populate the `TaskRun` status with a timeout message, the Tekton +entrypoint binary writes a timeout `Result` of the `InternalTektonResultType` +kind. Based on this `Result`, the `TaskRun` status is [populated](#Proposal) +while the `Result` is filtered out from `Task` author related results (like +`PipelineResourceResults`) based on its kind. + +### Caveats + +#### Resolution of a `Step` Timeout + +The resolution at which a `Step` timeout can be specified is the same as the +resolution of the [Duration +type](https://golang.org/pkg/time/#ParseDuration). The smallest resolution +supported by the Duration type is a nanosecond. Nevertheless, the +[motivation](#motivation) of this TEP is not to provide nanosecond resolution. +Instead, the aim is to provide a timeout that would [reasonably](#req2) meet +the `Task` authors expectations. E.g., a `Task` author may expect a `Step` to +execute for five seconds at most and therefore specify a six second timeout. + +Technically, a hard requirement on the resolution can not be set because +performance variability between cluster setups may introduce discrepancies. +However, as a reference, our tests have shown a resolution accuracy of about +10 ms on GKE clusters. This means that for a `Step` that has a 5 second +execution time, specifying a 5010 ms timeout will not cause a +timeout. On the other hand, a timeout specified between 5 seconds and 5010 ms +may cause the `Step` to timeout. Tekton tries to minimize overhead and therefore we do +not expect huge discrepancies with other clusters. + +## Test Plan + +* A unit test verifies the Tekton entrypoint binary can be timed out +* An integration test verifies a `Step` can be timed out +* An integration test verifies a timeout with a wide margin of 1 second will +not cause a `Step` timeout + - Concretely: This test will verify that a `Step` supposed to *sleep* for 1 + second will not timeout in case a 2 second `Step` timeout has been + specified + +## References + +[Issue #1690](https://github.com/tektoncd/pipeline/issues/1690) +[PR #3087](https://github.com/tektoncd/pipeline/pull/3087) diff --git a/bots/tep-automation/pkg/tep/types.go b/bots/tep-automation/pkg/tep/types.go new file mode 100644 index 0000000000..5fd9f06ccf --- /dev/null +++ b/bots/tep-automation/pkg/tep/types.go @@ -0,0 +1,95 @@ +package tep + +import ( + "bytes" + "fmt" + "text/template" + "time" +) + +const ( + // ActionParamName is the param name that will hold the pull request webhook action + ActionParamName = "pullRequestAction" + // PRNumberParamName is the param name that will hold the pull request number + PRNumberParamName = "pullRequestNumber" + // PRTitleParamName is the param name that will hold the pull request title + PRTitleParamName = "pullRequestTitle" + // PRBodyParamName is the param name that will hold the pull request body + PRBodyParamName = "pullRequestBody" + // PackageParamName is the param name that will hold the pull request's repository owner/name + PackageParamName = "package" + // PRIsMergedParamName is the param name that will hold whether the pull request is merged + PRIsMergedParamName = "pullRequestIsMerged" + // GitRevisionParamName is the param name that will hold the HEAD git SHA for the PR + GitRevisionParamName = "gitRevision" + + // CommunityBlobBaseURL is the base URL for links to blobs in github.com/tektoncd/community/teps + CommunityBlobBaseURL = "https://github.com/tektoncd/community/blob/main/teps/" + + // TrackingIssueBodyTmpl is the template (using text/template) for the body of tracking issues. + TrackingIssueBodyTmpl = `This issue tracks TEP-{{ .issue.TEPID }}. + +Use this issue for discussion of this TEP not directly related to pull requests updating or implementing the TEP. + +TEP: ({{ .tepURL }}) +Current status: {{ .mdStatus }} +Authors:{{ range .issue.Assignees }} +- @{{ . }}{{ end }} +TEP PRs:{{ range .issue.TEPPRs }} +- (https://github.com/tektoncd/community/pull/{{ . }}){{ end }} +Implementation PRs:{{ range .issue.ImplementationPRs }} +- (https://github.com/tektoncd/{{ .Repo }}/pull/{{ .Number }}){{ end }} +` +) + +// CommentInfo stores the PR comment ID for a comment the notifier made about a TEP, the TEP's status when the +// comment was made, and the TEP ID +type CommentInfo struct { + CommentID int64 + TEPs []string + ToImplemented bool +} + +// TEPInfo stores information about a TEP parsed from https://github.com/tektoncd/community/blob/main/teps/README.md +type TEPInfo struct { + ID string + Title string + Status Status + Filename string + LastModified time.Time + Authors []string +} + +// ImplementationPR stores a TEP implementation PR's repository and PR number +type ImplementationPR struct { + Repo string + Number int +} + +// TrackingIssue stores information about a TEP tracking issue in the community repo. +type TrackingIssue struct { + IssueNumber int + IssueState string + TEPStatus Status + TEPID string + TEPPRs []int + ImplementationPRs []ImplementationPR + Assignees []string +} + +// GetBody returns the body content for this tracking issue. +func (ti TrackingIssue) GetBody(filename string) (string, error) { + data := map[string]interface{}{ + "issue": ti, + "tepURL": fmt.Sprintf("%s%s", CommunityBlobBaseURL, filename), + "mdStatus": ti.TEPStatus.ForMarkdown(), + } + + buf := bytes.NewBufferString("") + if bodyTmpl, err := template.New("trackingIssueBody").Parse(TrackingIssueBodyTmpl); err != nil { + return "", fmt.Errorf("failed to parse template for tracking issue body: %v", err) + } else if err := bodyTmpl.Execute(buf, data); err != nil { + return "", fmt.Errorf("failed to execute template for tracking issue body: %v", err) + } + return buf.String(), nil +} diff --git a/bots/tep-automation/pkg/tep/types_test.go b/bots/tep-automation/pkg/tep/types_test.go new file mode 100644 index 0000000000..b87e5c0d1e --- /dev/null +++ b/bots/tep-automation/pkg/tep/types_test.go @@ -0,0 +1,53 @@ +package tep_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" +) + +func TestTrackingIssue_GetBody(t *testing.T) { + ti := tep.TrackingIssue{ + IssueNumber: 5, + IssueState: "open", + TEPStatus: tep.ProposedStatus, + TEPID: "1234", + TEPPRs: []int{100, 120}, + ImplementationPRs: []tep.ImplementationPR{ + { + Repo: "pipeline", + Number: 77, + }, + { + Repo: "triggers", + Number: 88, + }, + }, + Assignees: []string{"abayer", "vdemeester"}, + } + + mdFilename := "1234-some-tep.md" + + expected := `This issue tracks TEP-1234. + +Use this issue for discussion of this TEP not directly related to pull requests updating or implementing the TEP. + +TEP: (https://github.com/tektoncd/community/blob/main/teps/1234-some-tep.md) +Current status: ` + "`proposed`" + ` +Authors: +- @abayer +- @vdemeester +TEP PRs: +- (https://github.com/tektoncd/community/pull/100) +- (https://github.com/tektoncd/community/pull/120) +Implementation PRs: +- (https://github.com/tektoncd/pipeline/pull/77) +- (https://github.com/tektoncd/triggers/pull/88) +` + + body, err := ti.GetBody(mdFilename) + require.NoError(t, err) + assert.Equal(t, expected, body) +} diff --git a/bots/tep-automation/pkg/testutil/util.go b/bots/tep-automation/pkg/testutil/util.go new file mode 100644 index 0000000000..83a17a18d3 --- /dev/null +++ b/bots/tep-automation/pkg/testutil/util.go @@ -0,0 +1,135 @@ +package testutil + +import ( + "encoding/base64" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/tep" + + "github.com/google/go-github/v41/github" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/plumbing/bots/tep-automation/pkg/ghclient" +) + +const ( + // DefaultTEPReadmeContent is used in tests that expect a consistent README.md + DefaultTEPReadmeContent = `there are three teps in here +on later lines +|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-20 | +|[TEP-5678](5678-second-one.md) | Another TEP Title | proposed | 2021-12-20 | +|[TEP-4321](4321-third-one.md) | Yet Another TEP Title | implementing | 2021-12-20 | +tada, three valid TEPs +` +) + +var ( + // ReadmeURL is used for configuring handlers expecting to get the contents of the TEP index README. + ReadmeURL = fmt.Sprintf("/repos/%s/%s/contents/%s/%s", ghclient.TEPsOwner, ghclient.TEPsRepo, + ghclient.TEPsDirectory, ghclient.TEPsReadmeFile) + + defaultRunParams = map[string]string{ + tep.ActionParamName: "opened", + tep.PRNumberParamName: "1", + tep.PRTitleParamName: "Some PR", + tep.PRBodyParamName: "A PR body, without any TEPs in it", + tep.PackageParamName: "tektoncd/pipeline", + tep.PRIsMergedParamName: "false", + tep.GitRevisionParamName: "someSha", + } +) + +// DefaultREADMEHandlerFunc returns a function that serves the default readme content +func DefaultREADMEHandlerFunc() func(http.ResponseWriter, *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + _, _ = fmt.Fprint(w, GHContentJSON(DefaultTEPReadmeContent)) + } +} + +// NoCommentsOnPRHandlerFunc returns a function that handles returning no comments for a PR, or posting a new comment +func NoCommentsOnPRHandlerFunc(t *testing.T) func(http.ResponseWriter, *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case "GET": + _, _ = fmt.Fprint(w, `[]`) + return + case "POST": + _, _ = fmt.Fprint(w, `{"id":1}`) + return + default: + t.Errorf("unexpected method %s", r.Method) + } + } +} + +// ConstructRunParams generates a slice of Params from a map of overrides of default parameters and a second map of additional parameters +func ConstructRunParams(overrides map[string]string, additionalParams map[string]string) []v1beta1.Param { + var params []v1beta1.Param + + for key, defaultValue := range defaultRunParams { + if overrideValue, ok := overrides[key]; ok { + if overrideValue != "" { + params = append(params, v1beta1.Param{ + Name: key, + Value: v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: overrideValue}, + }) + } + } else { + params = append(params, v1beta1.Param{ + Name: key, + Value: v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: defaultValue}, + }) + } + } + + for k, v := range additionalParams { + params = append(params, v1beta1.Param{ + Name: k, + Value: v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: v}, + }) + } + + return params +} + +// GHContentJSON takes a string and returns the string of JSON we'd expect from GitHub for a file's contents, using a +// default filename and path +func GHContentJSON(content string) string { + return GHContentJSONDetailed(content, "SOMEFILE", "SOMEPATH") +} + +// GHContentJSONDetailed takes a string, a filename, and a path, and returns the string of JSON we'd expect from GitHub for a file's contents +func GHContentJSONDetailed(content, filename, path string) string { + encContent := base64.StdEncoding.EncodeToString([]byte(content)) + + return fmt.Sprintf(`{ + "type": "file", + "content": "%s", + "encoding": "base64", + "size": 1234, + "name": "%s", + "path": "%s" + }`, encContent, filename, path) +} + +// SetupFakeGitHub configures a fake GitHub server and returns it +func SetupFakeGitHub() (*github.Client, *http.ServeMux, func()) { + apiPath := "/api-v3" + + mux := http.NewServeMux() + + handler := http.NewServeMux() + handler.Handle(apiPath+"/", http.StripPrefix(apiPath, mux)) + + server := httptest.NewServer(handler) + + client := github.NewClient(nil) + ghURL, _ := url.Parse(server.URL + apiPath + "/") + client.BaseURL = ghURL + client.UploadURL = ghURL + + return client, mux, server.Close +} diff --git a/tep-notifier/tekton/README.md b/bots/tep-automation/tekton/README.md similarity index 100% rename from tep-notifier/tekton/README.md rename to bots/tep-automation/tekton/README.md diff --git a/tep-notifier/tekton/kustomization.yaml b/bots/tep-automation/tekton/kustomization.yaml similarity index 100% rename from tep-notifier/tekton/kustomization.yaml rename to bots/tep-automation/tekton/kustomization.yaml diff --git a/tep-notifier/tekton/publish.yaml b/bots/tep-automation/tekton/publish.yaml similarity index 100% rename from tep-notifier/tekton/publish.yaml rename to bots/tep-automation/tekton/publish.yaml diff --git a/tep-notifier/tekton/release-pipeline.yaml b/bots/tep-automation/tekton/release-pipeline.yaml similarity index 100% rename from tep-notifier/tekton/release-pipeline.yaml rename to bots/tep-automation/tekton/release-pipeline.yaml diff --git a/tekton/ci/shared/bindings.yaml b/tekton/ci/shared/bindings.yaml index af1118ab3f..db7d3db1e0 100644 --- a/tekton/ci/shared/bindings.yaml +++ b/tekton/ci/shared/bindings.yaml @@ -105,3 +105,5 @@ spec: value: $(body.repository.full_name) - name: pullRequestIsMerged value: $(body.pull_request.merged) + - name: gitRevision + value: $(body.pull_request.head.sha) diff --git a/tep-notifier/pkg/reconciler/reconciler.go b/tep-notifier/pkg/reconciler/reconciler.go deleted file mode 100644 index dbe5b2ff0e..0000000000 --- a/tep-notifier/pkg/reconciler/reconciler.go +++ /dev/null @@ -1,599 +0,0 @@ -package reconciler - -import ( - "context" - "fmt" - "path/filepath" - "regexp" - "strconv" - "strings" - "time" - - "github.com/google/go-github/v41/github" - "github.com/pkg/errors" - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" - "golang.org/x/oauth2" - "knative.dev/pkg/logging" - kreconciler "knative.dev/pkg/reconciler" -) - -const ( - // TEPCommentAndStatusRegexFmt is the format used for adding the metadata for TEP and status to the comment. - TEPCommentAndStatusRegexFmt = "\n" - - // CommunityBlobBaseURL is the base URL for links to blobs in github.com/tektoncd/community/teps - CommunityBlobBaseURL = "https://github.com/tektoncd/community/blob/main/teps/" - - // TEPsOwner is the GitHub owner for the repo containing the TEPs. - TEPsOwner = "tektoncd" - // TEPsRepo is the GitHub repository containing the TEPs, under TEPsOwner. - TEPsRepo = "community" - // TEPsDirectory is the directory containing the TEPs, within TEPsRepo. - TEPsDirectory = "teps" - // TEPsBranch is the branch in TEPsRepo we will look at. - TEPsBranch = "main" - // TEPsReadmeFile is the filename to find the list of TEPs and statuses in. - TEPsReadmeFile = "README.md" - - // ProposedStatus is the "proposed" TEP status. - ProposedStatus TEPStatus = "proposed" - // ImplementableStatus is the "implementable" status. - ImplementableStatus TEPStatus = "implementable" - // ImplementingStatus is the "implementing" status. - ImplementingStatus TEPStatus = "implementing" - // ImplementedStatus is the "implemented" status. - ImplementedStatus TEPStatus = "implemented" - // WithdrawnStatus is the "withdrawn" status. - WithdrawnStatus TEPStatus = "withdrawn" - // ReplacedStatus is the "replaced" status. - ReplacedStatus TEPStatus = "replaced" - - // BotUser is the user we will be making comments with - BotUser = "tekton-robot" - - // ToImplementedCommentHeader is the header for PR comments reminding to update a given TEP from the - // `implementing` status to the `implemented` status. - ToImplementedCommentHeader = "This merged pull request appears to be referencing one or more [Tekton Enhancement Proposals](https://github.com/tektoncd/community/tree/main/teps#readme) " + - "which are currently in the `implementing` state. If this PR finished the work towards implementing these TEPs, " + - "please update their state(s) to `implemented`.\n" + - "\n" + - "TEPs:\n" - // ToImplementingCommentHeader is the header for PR comments reminding to update a given TEP from the - // `proposed` or `implementable` status to the `implementing` status. - ToImplementingCommentHeader = "This pull request appears to be referencing one or more [Tekton Enhancement Proposals](https://github.com/tektoncd/community/tree/main/teps#readme) " + - "which are currently in the `proposed` or `implementable` states. If this PR contains " + - "work towards implementing these TEPs, please update their state(s) to `implementing`.\n" + - "\n" + - "TEPs:\n" - - // ActionParamName is the param name that will hold the pull request webhook action - ActionParamName = "pullRequestAction" - // PRNumberParamName is the param name that will hold the pull request number - PRNumberParamName = "pullRequestNumber" - // PRTitleParamName is the param name that will hold the pull request title - PRTitleParamName = "pullRequestTitle" - // PRBodyParamName is the param name that will hold the pull request body - PRBodyParamName = "pullRequestBody" - // PackageParamName is the param name that will hold the pull request's repository owner/name - PackageParamName = "package" - // PRIsMergedParamName is the param name that will hold whether the pull request is merged - PRIsMergedParamName = "pullRequestIsMerged" - - closedAction = "closed" - openedAction = "opened" - editedAction = "edited" -) - -var ( - // TEPCommentAndStatusRegex is used to detect whether a comment already is being used for the reminder message for - // a particular TEP and status. - TEPCommentAndStatusRegex = regexp.MustCompile(``) - // NotifierActionRegex is used to detect whether a comment is referring to transitioning to implementing or to implemented. - NotifierActionRegex = regexp.MustCompile(``) - // TEPRegex is used to parse out "TEP-1234" from PR bodies and titles. - TEPRegex = regexp.MustCompile(`TEP-(\d+)`) - // TEPURLRegex is used to parse out TEP URLs, like https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md, from - // PR bodies. We ignore the branch when looking for matches. - TEPURLRegex = regexp.MustCompile(`https://github\.com/tektoncd/community/blob/.*?/teps/(\d+)-.*?\.md`) - // TEPsInReadme matches rows in the table in https://github.com/tektoncd/community/blob/main/teps/README.md - TEPsInReadme = regexp.MustCompile(`\|\[TEP-(\d+)]\((.*?\.md)\) \| (.*?) \| (.*?) \| (\d\d\d\d-\d\d-\d\d) \|`) - - // TEPStatuses is a list of all valid TEP statuses - TEPStatuses = []TEPStatus{ - ProposedStatus, - ImplementableStatus, - ImplementingStatus, - ImplementedStatus, - WithdrawnStatus, - ReplacedStatus, - } -) - -// TEPStatus is a valid TEP status -type TEPStatus string - -// TEPCommentInfo stores the PR comment ID for a comment the notifier made about a TEP, the TEP's status when the -// comment was made, and the TEP ID -type TEPCommentInfo struct { - CommentID int64 - TEPs []string - ToImplemented bool -} - -// TEPInfo stores information about a TEP parsed from https://github.com/tektoncd/community/blob/main/teps/README.md -type TEPInfo struct { - ID string - Title string - Status TEPStatus - Filename string - LastModified time.Time -} - -// Reconciler handles the actual parsing of PR title and description for TEP identifiers, and adds comments to the PR -// as appropriate. -type Reconciler struct { - GHClient *github.Client -} - -// NewReconciler creates a new Reconciler instance configured with a GitHub client -func NewReconciler(ctx context.Context, ghToken string) *Reconciler { - // Allow anonymous for testing purposes - if ghToken == "" { - return &Reconciler{ - GHClient: github.NewClient(nil), - } - } - - ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: ghToken}) - tc := oauth2.NewClient(ctx, ts) - - return &Reconciler{ - GHClient: github.NewClient(tc), - } -} - -// ReconcileKind implements Interface.ReconcileKind. -func (r *Reconciler) ReconcileKind(ctx context.Context, run *v1alpha1.Run) kreconciler.Event { - logger := logging.FromContext(ctx) - logger.Infof("Reconciling %s/%s", run.Namespace, run.Name) - - // Ignore completed waits. - if run.IsDone() { - logger.Info("Run is finished, done reconciling") - return nil - } - - if run.Spec.Ref == nil || - run.Spec.Ref.APIVersion != v1beta1.SchemeGroupVersion.String() || run.Spec.Ref.Kind != Kind { - logger.Warnf("Should not have been notified about Run %s/%s; will do nothing", run.Namespace, run.Name) - return nil - } - - prActionParam := run.Spec.GetParam(ActionParamName) - if prActionParam == nil || prActionParam.Value.StringVal == "" { - run.Status.MarkRunFailed("MissingPullRequestAction", "The %s param was not passed", ActionParamName) - return nil - } - prAction := prActionParam.Value.StringVal - - // Short-circuit for PR actions other than `closed`, `edited`, or `opened`. - if prAction != closedAction && prAction != editedAction && prAction != openedAction { - logger.Warnf("Ignoring PR action %s; will do nothing", prAction) - return nil - } - - prNumberParam := run.Spec.GetParam(PRNumberParamName) - if prNumberParam == nil || prNumberParam.Value.StringVal == "" { - run.Status.MarkRunFailed("MissingPullRequestNumber", "The %s param was not passed", PRNumberParamName) - return nil - } - prNumber, err := strconv.Atoi(prNumberParam.Value.StringVal) - if err != nil { - run.Status.MarkRunFailed("InvalidPullRequestNumber", "%s is not a valid value for the %s param", prNumberParam.Value.StringVal, PRNumberParamName) - return nil - } - - prTitleParam := run.Spec.GetParam(PRTitleParamName) - if prTitleParam == nil || prTitleParam.Value.StringVal == "" { - run.Status.MarkRunFailed("MissingPullRequestTitle", "The %s param was not passed", PRTitleParamName) - return nil - } - prTitle := prTitleParam.Value.StringVal - - prBodyParam := run.Spec.GetParam(PRBodyParamName) - if prBodyParam == nil || prBodyParam.Value.StringVal == "" { - run.Status.MarkRunFailed("MissingPullRequestBody", "The %s param was not passed", PRBodyParamName) - return nil - } - prBody := prBodyParam.Value.StringVal - - orgAndRepoParam := run.Spec.GetParam(PackageParamName) - if orgAndRepoParam == nil || orgAndRepoParam.Value.StringVal == "" { - run.Status.MarkRunFailed("MissingPackage", "The %s param was not passed", PackageParamName) - return nil - } - splitOrgAndRepo := strings.Split(orgAndRepoParam.Value.StringVal, "/") - if len(splitOrgAndRepo) < 2 { - run.Status.MarkRunFailed("InvalidPackage", "The %s param value %s does not contain an owner and a repository seperated by '/'", - PackageParamName, orgAndRepoParam.Value.StringVal) - return nil - } - repo := splitOrgAndRepo[1] - - isMergedParam := run.Spec.GetParam(PRIsMergedParamName) - if isMergedParam == nil || isMergedParam.Value.StringVal == "" { - run.Status.MarkRunFailed("MissingPullRequestIsMerged", "The %s param was not passed", PRIsMergedParamName) - return nil - } - isMerged, err := strconv.ParseBool(isMergedParam.Value.StringVal) - if err != nil { - run.Status.MarkRunFailed("InvalidPullRequestIsMerged", "%s is not a valid value for the %s param", isMergedParam.Value.StringVal, PRIsMergedParamName) - return nil - } - - // Short-circuit if the action is "closed" but the PR is not merged. - if prAction == closedAction && !isMerged { - logger.Warn("Ignoring closed PR because the PR was not merged; will do nothing") - return nil - } - - if len(run.Spec.Params) > 6 { - var found []string - for _, p := range run.Spec.Params { - if p.Name == ActionParamName || - p.Name == PRNumberParamName || - p.Name == PRTitleParamName || - p.Name == PRBodyParamName || - p.Name == PackageParamName || - p.Name == PRIsMergedParamName { - continue - } - found = append(found, p.Name) - } - run.Status.MarkRunFailed("UnexpectedParams", "Found unexpected params: %v", found) - return nil - } - - tepsOnPR, err := r.TEPsInPR(ctx, prTitle, prBody) - if err != nil { - run.Status.MarkRunFailed("LoadingPRTEPs", "Failure loading TEPs for %s/%s PR #%d: %s", TEPsOwner, repo, prNumber, err) - return nil - } - - var tepsForComment []TEPInfo - var commentFunc func([]TEPInfo) string - var transitionStates []TEPStatus - - if prAction == closedAction { - commentFunc = PRMergedComment - transitionStates = append(transitionStates, ImplementingStatus) - } else { - commentFunc = PROpenedComment - transitionStates = append(transitionStates, ProposedStatus, ImplementableStatus) - } - - for _, tep := range tepsOnPR { - shouldInclude := false - for _, ts := range transitionStates { - if ts == tep.Status { - shouldInclude = true - } - } - - if shouldInclude { - tepsForComment = append(tepsForComment, tep) - } - } - - if len(tepsForComment) > 0 { - commentBody := commentFunc(tepsForComment) - - existingComments, err := r.TEPCommentsOnPR(ctx, repo, prNumber) - if err != nil { - run.Status.MarkRunFailed("CheckingPRComments", "Failure checking for TEP comments for %s/%s PR #%d: %s", TEPsOwner, repo, prNumber, err) - return nil - } - - var commentToUpdate *TEPCommentInfo - for _, cmt := range existingComments { - if prAction == closedAction && cmt.ToImplemented { - commentToUpdate = &cmt - break - } else if prAction == editedAction || prAction == openedAction { - commentToUpdate = &cmt - } - } - - if commentToUpdate != nil { - needsUpdate := false - for _, newTep := range tepsForComment { - found := false - for _, id := range commentToUpdate.TEPs { - if id == newTep.ID { - found = true - break - } - } - if !found { - needsUpdate = true - break - } - } - - if needsUpdate { - err = r.EditComment(ctx, repo, commentToUpdate.CommentID, commentBody) - if err != nil { - run.Status.MarkRunFailed("UpdatingPRComment", "Failure updating existing comment %d for %s/%s PR #%d: %s", - commentToUpdate.CommentID, TEPsOwner, repo, prNumber, err) - return nil - } - run.Status.MarkRunSucceeded("CommentUpdated", "Existing comment %d for %s/%s PR #%d updated", - commentToUpdate.CommentID, TEPsOwner, repo, prNumber) - return nil - } - } else { - err = r.AddComment(ctx, repo, prNumber, commentBody) - if err != nil { - run.Status.MarkRunFailed("AddingPRComment", "Failure adding new comment for %s/%s PR #%d: %s", - TEPsOwner, repo, prNumber, err) - return nil - } - run.Status.MarkRunSucceeded("CommentAdded", "Comment for %s/%s PR #%d", - TEPsOwner, repo, prNumber) - return nil - } - } - - // If we got here, then we didn't need to do anything. - logger.Warnf("No TEPs found in title or body for %s/%s PR #%d; nothing to do", TEPsOwner, repo, prNumber) - return nil -} - -// TEPCommentsOnPR finds all comments on the given PR made by this notifier -func (r *Reconciler) TEPCommentsOnPR(ctx context.Context, repo string, prNumber int) ([]TEPCommentInfo, error) { - listOpts := &github.IssueListCommentsOptions{ - ListOptions: github.ListOptions{ - PerPage: 20, - }, - } - - var tepComments []TEPCommentInfo - - for { - comments, resp, err := r.GHClient.Issues.ListComments(ctx, TEPsOwner, repo, prNumber, listOpts) - if err != nil { - return nil, errors.Wrapf(err, "getting comments for PR #%d in %s/%s", prNumber, TEPsOwner, repo) - } - - for _, c := range comments { - if c.ID != nil && c.Body != nil && c.User != nil && c.User.Login != nil && *c.User.Login == BotUser { - tepsOnComment, toImplemented := GetTEPCommentDetails(*c.Body) - - if len(tepsOnComment) > 0 { - tci := TEPCommentInfo{ - CommentID: *c.ID, - TEPs: []string{}, - ToImplemented: toImplemented, - } - for tID, tStatus := range tepsOnComment { - if !isValidStatus(tStatus) { - return nil, fmt.Errorf("metadata for TEP-%s has invalid status %s", tID, tStatus) - } - tci.TEPs = append(tci.TEPs, tID) - } - tepComments = append(tepComments, tci) - } - } - } - - if resp.NextPage == 0 { - break - } - listOpts.Page = resp.NextPage - } - - return tepComments, nil -} - -// GetTEPsFromReadme fetches https://github.com/tektoncd/community/blob/main/teps/README.md, parses out the TEPs from -// the table in that file, and returns a map of TEP IDs (i.e., "1234") to TEPInfo for that TEP. -func (r *Reconciler) GetTEPsFromReadme(ctx context.Context) (map[string]TEPInfo, error) { - fc, _, _, err := r.GHClient.Repositories.GetContents(ctx, TEPsOwner, TEPsRepo, filepath.Join(TEPsDirectory, TEPsReadmeFile), &github.RepositoryContentGetOptions{ - Ref: TEPsBranch, - }) - if err != nil { - return nil, errors.Wrapf(err, "fetching contents of https://github.com/%s/%s/blob/%s/%s/%s", TEPsOwner, TEPsRepo, - TEPsBranch, TEPsDirectory, TEPsReadmeFile) - } - - readmeStr, err := fc.GetContent() - if err != nil { - return nil, errors.Wrapf(err, "reading content of https://github.com/%s/%s/blob/%s/%s/%s", TEPsOwner, TEPsRepo, - TEPsBranch, TEPsDirectory, TEPsReadmeFile) - } - - teps, err := ExtractTEPsFromReadme(readmeStr) - if err != nil { - return nil, errors.Wrapf(err, "parsing content of https://github.com/%s/%s/blob/%s/%s/%s", TEPsOwner, TEPsRepo, - TEPsBranch, TEPsDirectory, TEPsReadmeFile) - } - - return teps, nil -} - -// TEPsInPR returns all TEPs referenced in the PR title or body in a map with the TEP ID as key and information about the -// TEP as value. -func (r *Reconciler) TEPsInPR(ctx context.Context, prTitle, prBody string) (map[string]TEPInfo, error) { - tepsWithInfo := make(map[string]TEPInfo) - - tepIDs := GetTEPIDsFromPR(prTitle, prBody) - - if len(tepIDs) == 0 { - return tepsWithInfo, nil - } - - allTEPs, err := r.GetTEPsFromReadme(ctx) - if err != nil { - return nil, err - } - - for _, tID := range tepIDs { - tepsWithInfo[tID] = allTEPs[tID] - } - - return tepsWithInfo, nil -} - -// AddComment adds a new comment to the PR -func (r *Reconciler) AddComment(ctx context.Context, repo string, prNumber int, body string) error { - input := &github.IssueComment{ - Body: github.String(body), - } - _, _, err := r.GHClient.Issues.CreateComment(ctx, TEPsOwner, repo, prNumber, input) - return err -} - -// EditComment updates an existing comment on the PR -func (r *Reconciler) EditComment(ctx context.Context, repo string, commentID int64, body string) error { - input := &github.IssueComment{ - Body: github.String(body), - } - _, _, err := r.GHClient.Issues.EditComment(ctx, TEPsOwner, repo, commentID, input) - return err -} - -// GetTEPCommentDetails looks at a PR comment and extracts any TEP IDs and their status in the comment and whether this -// comment is for transitioning TEPs to `implementing` or to `implemented` -func GetTEPCommentDetails(comment string) (map[string]string, bool) { - teps := make(map[string]string) - - toImplemented := false - notifierMatch := NotifierActionRegex.FindStringSubmatch(comment) - if len(notifierMatch) > 1 && notifierMatch[1] == string(ImplementedStatus) { - toImplemented = true - } - - for _, m := range TEPCommentAndStatusRegex.FindAllStringSubmatch(comment, -1) { - if len(m) > 2 { - teps[m[1]] = m[2] - } - } - - return teps, toImplemented -} - -// GetTEPIDsFromPR extracts all TEP IDs and URLs in the given PR title and body -func GetTEPIDsFromPR(prTitle, prBody string) []string { - var tepIDs []string - - // Find "TEP-1234" in PR title - for _, m := range TEPRegex.FindAllStringSubmatch(prTitle, -1) { - if len(m) > 1 { - tepIDs = append(tepIDs, m[1]) - } - } - // Find TEP URLs in title - for _, m := range TEPURLRegex.FindAllStringSubmatch(prTitle, -1) { - if len(m) > 1 { - tepIDs = append(tepIDs, m[1]) - } - } - - // Find "TEP-1234" in PR body - for _, m := range TEPRegex.FindAllStringSubmatch(prBody, -1) { - if len(m) > 1 { - tepIDs = append(tepIDs, m[1]) - } - } - // Find TEP URLs in body - for _, m := range TEPURLRegex.FindAllStringSubmatch(prBody, -1) { - if len(m) > 1 { - tepIDs = append(tepIDs, m[1]) - } - } - - return tepIDs -} - -// ExtractTEPsFromReadme takes the body of https://github.com/tektoncd/community/blob/main/teps/README.md and extracts a -// map of all TEP IDs (i.e., "1234" for TEP-1234) and their statuses. -func ExtractTEPsFromReadme(readmeBody string) (map[string]TEPInfo, error) { - teps := make(map[string]TEPInfo) - - for _, m := range TEPsInReadme.FindAllStringSubmatch(readmeBody, -1) { - if len(m) > 5 { - // TODO(abayer): For some reason, I can't ever get time.Parse to handle a format of "2021-12-20" so let's just pad it and do it as RFC3339. - lastMod, err := time.Parse(time.RFC3339, fmt.Sprintf("%sT00:00:00Z", m[5])) - if err != nil { - return nil, err - } - - if !isValidStatus(m[4]) { - return nil, fmt.Errorf("%s is not a valid status", m[4]) - } - teps[m[1]] = TEPInfo{ - ID: m[1], - Title: m[3], - Status: TEPStatus(m[4]), - Filename: m[2], - LastModified: lastMod, - } - } - } - - return teps, nil -} - -// GetTEPsWithStatus filters a map of TEP ID to TEPInfo for all TEPs in a given status -func GetTEPsWithStatus(input map[string]TEPInfo, desiredStatus TEPStatus) map[string]TEPInfo { - teps := make(map[string]TEPInfo) - - for k, v := range input { - if v.Status == desiredStatus { - teps[k] = v - } - } - - return teps -} - -// PROpenedComment returns the appropriate GitHub Markdown-formatted content for the PR comment on opened/edited PRs -// referencing TEPs in `proposed` or `implementable` states. -func PROpenedComment(teps []TEPInfo) string { - return generatePRComment(ToImplementingCommentHeader, teps) -} - -// PRMergedComment returns the appropriate GitHub Markdown-formatted content for the PR comment on merged PRs -// referencing TEPs in the `implementing` state. -func PRMergedComment(teps []TEPInfo) string { - return generatePRComment(ToImplementedCommentHeader, teps) -} - -func generatePRComment(header string, teps []TEPInfo) string { - var commentLines []string - var listLines []string - var metadataLines []string - - for _, tep := range teps { - listLines = append(listLines, fmt.Sprintf(" * [TEP-%s (%s)](%s%s), current status: `%s`\n", tep.ID, tep.Title, CommunityBlobBaseURL, tep.Filename, tep.Status)) - metadataLines = append(metadataLines, fmt.Sprintf(TEPCommentAndStatusRegexFmt, tep.ID, tep.Status)) - } - - commentLines = append(commentLines, header) - commentLines = append(commentLines, listLines...) - commentLines = append(commentLines, "\n") // Newline between the list of TEPs and the metadata - commentLines = append(commentLines, metadataLines...) - - return strings.Join(commentLines, "") // TODO(abayer): Probably should get rid of trailing newlines on the header and the list/metadata lines and just join on "\n" here. -} - -func isValidStatus(s string) bool { - for _, status := range TEPStatuses { - if s == string(status) { - return true - } - } - - return false -} diff --git a/tep-notifier/pkg/reconciler/reconciler_test.go b/tep-notifier/pkg/reconciler/reconciler_test.go deleted file mode 100644 index c28e570306..0000000000 --- a/tep-notifier/pkg/reconciler/reconciler_test.go +++ /dev/null @@ -1,1063 +0,0 @@ -package reconciler_test - -import ( - "context" - "encoding/base64" - "encoding/json" - "fmt" - "io/ioutil" - "net/http" - "net/http/httptest" - "net/url" - "strings" - "testing" - "time" - - "github.com/google/go-cmp/cmp" - "github.com/google/go-github/v41/github" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" - "github.com/tektoncd/plumbing/tep-notifier/pkg/reconciler" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "knative.dev/pkg/apis" -) - -const ( - defaultTEPReadmeContent = `there are three teps in here -on later lines -|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-20 | -|[TEP-5678](5678-second-one.md) | Another TEP Title | proposed | 2021-12-20 | -|[TEP-4321](4321-third-one.md) | Yet Another TEP Title | implementing | 2021-12-20 | -tada, three valid TEPs -` -) - -var ( - readmeURL = fmt.Sprintf("/repos/%s/%s/contents/%s/%s", reconciler.TEPsOwner, reconciler.TEPsRepo, - reconciler.TEPsDirectory, reconciler.TEPsReadmeFile) - defaultRunParams = map[string]string{ - reconciler.ActionParamName: "opened", - reconciler.PRNumberParamName: "1", - reconciler.PRTitleParamName: "Some PR", - reconciler.PRBodyParamName: "A PR body, without any TEPs in it", - reconciler.PackageParamName: "tektoncd/pipeline", - reconciler.PRIsMergedParamName: "false", - } -) - -func TestExtractTEPsFromReadme(t *testing.T) { - testCases := []struct { - name string - body string - expected map[string]reconciler.TEPInfo - errStr string - }{ - { - name: "no TEPs", - body: "there are no teps here", - expected: make(map[string]reconciler.TEPInfo), - }, - { - name: "single TEP", - body: `there's one tep in here -on a later line -|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-20 | -|[TEP-5678](5678-not-valid-line.md) | | proposed | 2021-12-20 | -tada, a single valid TEP and a bogus line -`, - expected: map[string]reconciler.TEPInfo{ - "1234": { - ID: "1234", - Title: "Some TEP Title", - Status: reconciler.ProposedStatus, - Filename: "1234-something-or-other.md", - LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), - }, - }, - }, - { - name: "multiple TEPs", - body: `there are two teps in here -on later lines -|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-20 | -|[TEP-5678](5678-valid-line-this-time.md) | A Second TEP Title | implemented | 2021-12-29 | -tada, a single valid TEP and a bogus line -`, - expected: map[string]reconciler.TEPInfo{ - "1234": { - ID: "1234", - Title: "Some TEP Title", - Status: reconciler.ProposedStatus, - Filename: "1234-something-or-other.md", - LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), - }, - "5678": { - ID: "5678", - Title: "A Second TEP Title", - Status: reconciler.ImplementedStatus, - Filename: "5678-valid-line-this-time.md", - LastModified: time.Date(2021, time.December, 29, 0, 0, 0, 0, time.UTC), - }, - }, - }, - { - name: "invalid date", - body: "|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-40 |", - errStr: `parsing time "2021-12-40T00:00:00Z": day out of range`, - }, - { - name: "invalid status", - body: "|[TEP-1234](1234-something-or-other.md) | Some TEP Title | invalid-status | 2021-12-20 |", - errStr: "invalid-status is not a valid status", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - teps, err := reconciler.ExtractTEPsFromReadme(tc.body) - if tc.errStr != "" { - require.EqualError(t, err, tc.errStr) - } else { - require.NoError(t, err) - } - assert.Equal(t, tc.expected, teps) - }) - } -} - -func TestGetTEPIDsFromPR(t *testing.T) { - testCases := []struct { - name string - title string - body string - ids []string - }{ - { - name: "none", - title: "Some PR title", - body: "Some PR body", - }, - { - name: "one id in title", - title: "This implements TEP-1234 perhaps", - body: "Some PR body", - ids: []string{"1234"}, - }, - { - name: "one id in body", - title: "Some PR title", - body: "This implements TEP-1234 perhaps", - ids: []string{"1234"}, - }, - { - name: "one url in title", - title: "This is for https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md", - body: "Some PR body", - ids: []string{"0002"}, - }, - { - name: "one url in body", - title: "Some PR title", - body: "This is for https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md", - ids: []string{"0002"}, - }, - { - name: "one id in title, one url in body", - title: "This is for TEP-1234", - body: "This is for https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md", - ids: []string{"0002", "1234"}, - }, - { - name: "two urls in body", - title: "Some PR title", - body: "This is for https://github.com/tektoncd/community/blob/main/teps/0002-custom-tasks.md and https://github.com/tektoncd/community/blob/main/teps/0006-tekton-metrics.md", - ids: []string{"0002", "0006"}, - }, - { - name: "two ids in body", - title: "Some PR title", - body: "This implements TEP-1234 perhaps. And what the heck, TEP-5678 as well.", - ids: []string{"1234", "5678"}, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - found := reconciler.GetTEPIDsFromPR(tc.title, tc.body) - assert.ElementsMatch(t, tc.ids, found) - }) - } -} - -func TestGetTEPsWithStatus(t *testing.T) { - testCases := []struct { - name string - status reconciler.TEPStatus - input map[string]reconciler.TEPInfo - expectedIDs []string - }{ - { - name: "none", - status: reconciler.ProposedStatus, - input: map[string]reconciler.TEPInfo{ - "1234": { - ID: "1234", - Title: "Some TEP", - Status: reconciler.ImplementableStatus, - Filename: "1234.md", - LastModified: time.Time{}, - }, - }, - }, - { - name: "one match", - status: reconciler.ProposedStatus, - input: map[string]reconciler.TEPInfo{ - "1234": { - ID: "1234", - Title: "Some TEP", - Status: reconciler.ImplementableStatus, - Filename: "1234.md", - LastModified: time.Time{}, - }, - "4321": { - ID: "4321", - Title: "Some Other TEP", - Status: reconciler.ProposedStatus, - Filename: "4321.md", - LastModified: time.Time{}, - }, - }, - expectedIDs: []string{"4321"}, - }, - { - name: "two match", - status: reconciler.ProposedStatus, - input: map[string]reconciler.TEPInfo{ - "1234": { - ID: "1234", - Title: "Some TEP", - Status: reconciler.ImplementableStatus, - Filename: "1234.md", - LastModified: time.Time{}, - }, - "4321": { - ID: "4321", - Title: "Some Other TEP", - Status: reconciler.ProposedStatus, - Filename: "4321.md", - LastModified: time.Time{}, - }, - "5678": { - ID: "5678", - Title: "A Third TEP", - Status: reconciler.ImplementedStatus, - Filename: "5678.md", - LastModified: time.Time{}, - }, - "8765": { - ID: "8765", - Title: "A Fourth TEP", - Status: reconciler.ProposedStatus, - Filename: "8765.md", - LastModified: time.Time{}, - }, - }, - expectedIDs: []string{"4321", "8765"}, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - found := reconciler.GetTEPsWithStatus(tc.input, tc.status) - var foundIDs []string - for k := range found { - foundIDs = append(foundIDs, k) - } - assert.ElementsMatch(t, tc.expectedIDs, foundIDs) - }) - } -} - -func TestGetTEPsFromReadme(t *testing.T) { - testCases := []struct { - name string - respContent string - expectedTEPs map[string]reconciler.TEPInfo - }{ - { - name: "none", - respContent: ghContentJSON("nothing"), - expectedTEPs: make(map[string]reconciler.TEPInfo), - }, - { - name: "one TEP", - respContent: `there's one tep in here -on a later line -|[TEP-1234](1234-something-or-other.md) | Some TEP Title | proposed | 2021-12-20 | -|[TEP-5678](5678-not-valid-line.md) | | proposed | 2021-12-20 | -tada, a single valid TEP and a bogus line -`, - expectedTEPs: map[string]reconciler.TEPInfo{ - "1234": { - ID: "1234", - Title: "Some TEP Title", - Status: reconciler.ProposedStatus, - Filename: "1234-something-or-other.md", - LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), - }, - }, - }, - { - name: "three TEPs", - respContent: defaultTEPReadmeContent, - expectedTEPs: map[string]reconciler.TEPInfo{ - "1234": { - ID: "1234", - Title: "Some TEP Title", - Status: reconciler.ProposedStatus, - Filename: "1234-something-or-other.md", - LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), - }, - "5678": { - ID: "5678", - Title: "Another TEP Title", - Status: reconciler.ProposedStatus, - Filename: "5678-second-one.md", - LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), - }, - "4321": { - ID: "4321", - Title: "Yet Another TEP Title", - Status: reconciler.ImplementingStatus, - Filename: "4321-third-one.md", - LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), - }, - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - client, mux, closeFunc := setupFakeGitHub() - defer closeFunc() - - mux.HandleFunc(readmeURL, - func(w http.ResponseWriter, r *http.Request) { - if !strings.HasSuffix(r.RequestURI, fmt.Sprintf("?ref=%s", reconciler.TEPsBranch)) { - t.Errorf("expected request for branch %s, but URI was %s", reconciler.TEPsBranch, r.RequestURI) - } - _, _ = fmt.Fprint(w, ghContentJSON(tc.respContent)) - }) - - r := &reconciler.Reconciler{GHClient: client} - - ctx := context.Background() - - teps, err := r.GetTEPsFromReadme(ctx) - require.NoError(t, err) - - if d := cmp.Diff(tc.expectedTEPs, teps); d != "" { - t.Errorf("Wrong TEPs from README.md: (-want, +got): %s", d) - } - }) - } -} - -func TestGetTEPCommentDetails(t *testing.T) { - testCases := []struct { - name string - comment string - teps map[string]string - toImplemented bool - }{ - { - name: "none", - comment: "this has no TEPs", - teps: make(map[string]string), - }, - { - name: "one TEP", - comment: `this comment has some text -and it also has a TEP - - - - -`, - toImplemented: true, - teps: map[string]string{ - "1234": "implementing", - }, - }, - { - name: "two TEPs", - comment: `this comment has some text -and it also has two TEPs - - - - - -`, - teps: map[string]string{ - "1234": string(reconciler.ProposedStatus), - "5678": string(reconciler.ImplementableStatus), - }, - toImplemented: false, - }, - { - name: "duplicate TEP", - comment: `this comment has some text -and it also has one TEP, duplicated with a different status - - - - - -`, - teps: map[string]string{ - "1234": string(reconciler.ImplementableStatus), - }, - toImplemented: false, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - teps, toImplemented := reconciler.GetTEPCommentDetails(tc.comment) - if d := cmp.Diff(tc.teps, teps); d != "" { - t.Errorf("Wrong TEPs from comment: (-want, +got): %s", d) - } - assert.Equalf(t, tc.toImplemented, toImplemented, "expected toImplemented to be %t, but is %t", tc.toImplemented, toImplemented) - }) - } -} - -type testComment struct { - id int64 - user string - body string -} - -func TestTEPCommentsOnPR(t *testing.T) { - testCases := []struct { - name string - comments []testComment - expected []reconciler.TEPCommentInfo - }{ - { - name: "none with bot user comment", - comments: []testComment{{ - id: 1, - user: reconciler.BotUser, - body: "There are no TEPs here", - }}, - }, - { - name: "one with bot user comment", - comments: []testComment{{ - id: 1, - user: reconciler.BotUser, - body: `this comment has some text -and it also has a TEP - - - - -`, - }}, - expected: []reconciler.TEPCommentInfo{{ - CommentID: 1, - TEPs: []string{"1234"}, - ToImplemented: false, - }}, - }, - { - name: "both implementing and implemented comments", - comments: []testComment{ - { - id: 1, - user: reconciler.BotUser, - body: `this comment has some text -and it also has a TEP - - - - -`, - }, - { - id: 2, - user: reconciler.BotUser, - body: `close this TEP - - - - -`, - }, - }, - expected: []reconciler.TEPCommentInfo{ - { - CommentID: 1, - TEPs: []string{"1234"}, - ToImplemented: false, - }, - { - CommentID: 2, - TEPs: []string{"1234"}, - ToImplemented: true, - }, - }, - }, - { - name: "one with other user comment", - comments: []testComment{{ - id: 1, - user: "abayer", - body: `this comment has some text -and it also has a TEP - - -`, - }}, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - client, mux, closeFunc := setupFakeGitHub() - defer closeFunc() - - mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/issues/1/comments", reconciler.TEPsOwner, reconciler.TEPsRepo), - func(w http.ResponseWriter, r *http.Request) { - var ghComments []*github.IssueComment - - for _, c := range tc.comments { - cCopy := c - ghComments = append(ghComments, &github.IssueComment{ - ID: &cCopy.id, - User: &github.User{ - Login: &cCopy.user, - }, - Body: &cCopy.body, - }) - } - - respBody, err := json.Marshal(ghComments) - if err != nil { - t.Fatal("marshalling GitHub comments") - } - _, _ = fmt.Fprint(w, string(respBody)) - }) - - r := &reconciler.Reconciler{GHClient: client} - - ctx := context.Background() - - tepComments, err := r.TEPCommentsOnPR(ctx, reconciler.TEPsRepo, 1) - require.NoError(t, err) - - assert.ElementsMatch(t, tc.expected, tepComments) - }) - } -} - -func TestPROpenedComment(t *testing.T) { - teps := []reconciler.TEPInfo{ - { - ID: "1234", - Title: "Some TEP Title", - Status: reconciler.ProposedStatus, - Filename: "1234-something-or-other.md", - LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), - }, - { - ID: "5678", - Title: "Some Other TEP Title", - Status: reconciler.ImplementableStatus, - Filename: "5678-insert-filename-here.md", - LastModified: time.Date(2021, time.December, 21, 0, 0, 0, 0, time.UTC), - }, - } - - expectedComment := reconciler.ToImplementingCommentHeader + - " * [TEP-1234 (Some TEP Title)](https://github.com/tektoncd/community/blob/main/teps/1234-something-or-other.md), current status: `proposed`\n" + - " * [TEP-5678 (Some Other TEP Title)](https://github.com/tektoncd/community/blob/main/teps/5678-insert-filename-here.md), current status: `implementable`\n" + - "\n" + - "\n" + - "\n" - - assert.Equal(t, expectedComment, reconciler.PROpenedComment(teps)) -} - -func TestPRClosedComment(t *testing.T) { - teps := []reconciler.TEPInfo{ - { - ID: "1234", - Title: "Some TEP Title", - Status: reconciler.ImplementingStatus, - Filename: "1234-something-or-other.md", - LastModified: time.Date(2021, time.December, 20, 0, 0, 0, 0, time.UTC), - }, - { - ID: "5678", - Title: "Some Other TEP Title", - Status: reconciler.ImplementingStatus, - Filename: "5678-insert-filename-here.md", - LastModified: time.Date(2021, time.December, 21, 0, 0, 0, 0, time.UTC), - }, - } - - expectedComment := reconciler.ToImplementedCommentHeader + - " * [TEP-1234 (Some TEP Title)](https://github.com/tektoncd/community/blob/main/teps/1234-something-or-other.md), current status: `implementing`\n" + - " * [TEP-5678 (Some Other TEP Title)](https://github.com/tektoncd/community/blob/main/teps/5678-insert-filename-here.md), current status: `implementing`\n" + - "\n" + - "\n" + - "\n" - - assert.Equal(t, expectedComment, reconciler.PRMergedComment(teps)) -} - -func TestAddComment(t *testing.T) { - client, mux, closeFunc := setupFakeGitHub() - defer closeFunc() - - input := &github.IssueComment{ - Body: github.String("some body"), - } - - mux.HandleFunc("/repos/tektoncd/pipeline/issues/1/comments", - func(w http.ResponseWriter, r *http.Request) { - v := new(github.IssueComment) - require.NoError(t, json.NewDecoder(r.Body).Decode(v)) - - require.Equal(t, "POST", r.Method) - - if d := cmp.Diff(input, v); d != "" { - t.Errorf("difference in POST body: %s", d) - } - _, _ = fmt.Fprint(w, `{"id":1}`) - }) - - r := &reconciler.Reconciler{GHClient: client} - - ctx := context.Background() - - require.NoError(t, r.AddComment(ctx, "pipeline", 1, "some body")) -} - -func TestEditComment(t *testing.T) { - client, mux, closeFunc := setupFakeGitHub() - defer closeFunc() - - input := &github.IssueComment{ - Body: github.String("some new body"), - } - - mux.HandleFunc("/repos/tektoncd/pipeline/issues/comments/1", - func(w http.ResponseWriter, r *http.Request) { - v := new(github.IssueComment) - require.NoError(t, json.NewDecoder(r.Body).Decode(v)) - - require.Equal(t, "PATCH", r.Method) - - if d := cmp.Diff(input, v); d != "" { - t.Errorf("difference in PATCH body: %s", d) - } - _, _ = fmt.Fprint(w, `{"id":1}`) - }) - - r := &reconciler.Reconciler{GHClient: client} - - ctx := context.Background() - - require.NoError(t, r.EditComment(ctx, "pipeline", 1, "some new body")) -} - -func TestReconcileKind(t *testing.T) { - defaultKindRef := &v1beta1.TaskRef{ - APIVersion: v1beta1.SchemeGroupVersion.String(), - Kind: reconciler.Kind, - } - - testCases := []struct { - name string - paramOverrides map[string]string - additionalParams map[string]string - kindRef *v1beta1.TaskRef - requests map[string]func(w http.ResponseWriter, r *http.Request) - doesNothing bool - expectedStatus corev1.ConditionStatus - expectedReason string - expectedErr error - }{ - { - name: "no TEPs", - doesNothing: true, - }, - { - name: "invalid ref", - kindRef: &v1beta1.TaskRef{ - APIVersion: v1beta1.SchemeGroupVersion.String(), - Kind: "SomethingElse", - }, - doesNothing: true, - }, - { - name: "wrong action", - paramOverrides: map[string]string{ - reconciler.ActionParamName: "assigned", - }, - doesNothing: true, - }, - { - name: "closed but unmerged", - paramOverrides: map[string]string{ - reconciler.ActionParamName: "closed", - }, - doesNothing: true, - }, - { - name: "missing action", - paramOverrides: map[string]string{ - reconciler.ActionParamName: "", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "MissingPullRequestAction", - }, - { - name: "missing PR number", - paramOverrides: map[string]string{ - reconciler.PRNumberParamName: "", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "MissingPullRequestNumber", - }, - { - name: "missing PR title", - paramOverrides: map[string]string{ - reconciler.PRTitleParamName: "", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "MissingPullRequestTitle", - }, - { - name: "missing PR body", - paramOverrides: map[string]string{ - reconciler.PRBodyParamName: "", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "MissingPullRequestBody", - }, - { - name: "missing package", - paramOverrides: map[string]string{ - reconciler.PackageParamName: "", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "MissingPackage", - }, - { - name: "missing PR isMerged", - paramOverrides: map[string]string{ - reconciler.PRIsMergedParamName: "", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "MissingPullRequestIsMerged", - }, - { - name: "invalid PR number", - paramOverrides: map[string]string{ - reconciler.PRNumberParamName: "banana", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "InvalidPullRequestNumber", - }, - { - name: "invalid package", - paramOverrides: map[string]string{ - reconciler.PackageParamName: "not-owner-slash-repo", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "InvalidPackage", - }, - { - name: "invalid additional param", - additionalParams: map[string]string{ - "something": "or other", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "UnexpectedParams", - }, - { - name: "fetching README 404", - paramOverrides: map[string]string{ - reconciler.PRTitleParamName: "PR referencing TEP-1234", - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "LoadingPRTEPs", - }, - { - name: "fetching PR comments 404", - paramOverrides: map[string]string{ - reconciler.PRTitleParamName: "PR referencing TEP-1234", - }, - requests: map[string]func(w http.ResponseWriter, r *http.Request){ - readmeURL: defaultREADMEHandlerFunc(), - }, - expectedStatus: corev1.ConditionFalse, - expectedReason: "CheckingPRComments", - }, - { - name: "adding comment for opened PR", - paramOverrides: map[string]string{ - reconciler.PRTitleParamName: "PR referencing TEP-1234", - }, - requests: map[string]func(w http.ResponseWriter, r *http.Request){ - readmeURL: defaultREADMEHandlerFunc(), - "/repos/tektoncd/pipeline/issues/1/comments": noCommentsOnPRHandlerFunc(t), - }, - expectedStatus: corev1.ConditionTrue, - expectedReason: "CommentAdded", - }, - { - name: "editing comment for opened PR", - paramOverrides: map[string]string{ - reconciler.PRTitleParamName: "PR referencing TEP-1234", - reconciler.PRBodyParamName: "With a body referencing TEP-5678", - }, - requests: map[string]func(w http.ResponseWriter, r *http.Request){ - readmeURL: defaultREADMEHandlerFunc(), - "/repos/tektoncd/pipeline/issues/1/comments": func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "GET", r.Method) - - commentID := int64(1) - commentUser := reconciler.BotUser - commentBody := fmt.Sprintf("%s* [TEP-1234] (Some TEP Title)][https://github.com/tektoncd/community/blob/main/teps/1234-something-or-other.md),"+ - "current status: `proposed`\n\n\n", reconciler.ToImplementingCommentHeader) - comments := []*github.IssueComment{{ - ID: &commentID, - Body: &commentBody, - User: &github.User{ - Login: &commentUser, - }, - }} - respBody, err := json.Marshal(comments) - if err != nil { - t.Fatal("marshalling GitHub comments") - } - _, _ = fmt.Fprint(w, string(respBody)) - }, - "/repos/tektoncd/pipeline/issues/comments/1": func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "PATCH", r.Method) - body, err := ioutil.ReadAll(r.Body) - require.NoError(t, err) - require.Contains(t, string(body), "TEP-5678") - _, _ = fmt.Fprint(w, `{"id":1}`) - }, - }, - expectedStatus: corev1.ConditionTrue, - expectedReason: "CommentUpdated", - }, - { - name: "wrong state for opened PR", - paramOverrides: map[string]string{ - reconciler.PRTitleParamName: "PR referencing TEP-4321", - }, - requests: map[string]func(w http.ResponseWriter, r *http.Request){ - readmeURL: defaultREADMEHandlerFunc(), - "/repos/tektoncd/pipeline/issues/1/comments": noCommentsOnPRHandlerFunc(t), - }, - doesNothing: true, - }, - { - name: "wrong state for closed PR", - paramOverrides: map[string]string{ - reconciler.ActionParamName: "closed", - reconciler.PRTitleParamName: "PR referencing TEP-1234", - reconciler.PRIsMergedParamName: "true", - }, - requests: map[string]func(w http.ResponseWriter, r *http.Request){ - readmeURL: defaultREADMEHandlerFunc(), - "/repos/tektoncd/pipeline/issues/1/comments": noCommentsOnPRHandlerFunc(t), - }, - doesNothing: true, - }, - { - name: "adding comment for closed PR", - paramOverrides: map[string]string{ - reconciler.ActionParamName: "closed", - reconciler.PRTitleParamName: "PR referencing TEP-4321", - reconciler.PRIsMergedParamName: "true", - }, - requests: map[string]func(w http.ResponseWriter, r *http.Request){ - readmeURL: defaultREADMEHandlerFunc(), - "/repos/tektoncd/pipeline/issues/1/comments": func(w http.ResponseWriter, r *http.Request) { - switch r.Method { - case "GET": - commentID := int64(1) - commentUser := reconciler.BotUser - commentBody := fmt.Sprintf("%s* [TEP-4321] (Some TEP Title)][https://github.com/tektoncd/community/blob/main/teps/4321-something-or-other.md),"+ - "current status: `implementable`\n\n\n", reconciler.ToImplementingCommentHeader) - comments := []*github.IssueComment{{ - ID: &commentID, - Body: &commentBody, - User: &github.User{ - Login: &commentUser, - }, - }} - respBody, err := json.Marshal(comments) - if err != nil { - t.Fatal("marshalling GitHub comments") - } - _, _ = fmt.Fprint(w, string(respBody)) - return - case "POST": - _, _ = fmt.Fprint(w, `{"id":1}`) - return - default: - t.Errorf("unexpected method %s", r.Method) - } - }, - }, - expectedStatus: corev1.ConditionTrue, - expectedReason: "CommentAdded", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - - ghClient, mux, closeFunc := setupFakeGitHub() - defer closeFunc() - - for k, v := range tc.requests { - mux.HandleFunc(k, v) - } - - r := &reconciler.Reconciler{GHClient: ghClient} - - run := &v1alpha1.Run{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-reconcile-run", - Namespace: "foo", - }, - Spec: v1alpha1.RunSpec{ - Params: constructRunParams(tc.paramOverrides, tc.additionalParams), - }, - } - if tc.kindRef != nil { - run.Spec.Ref = tc.kindRef - } else { - run.Spec.Ref = defaultKindRef - } - - err := r.ReconcileKind(ctx, run) - if tc.expectedErr != nil { - assert.Equal(t, tc.expectedErr, err) - } else { - require.NoError(t, err) - condition := run.Status.GetCondition(apis.ConditionSucceeded) - if tc.doesNothing { - assert.Nil(t, condition) - } else { - require.NotNil(t, condition, "Condition missing in Run") - - if condition.Status != tc.expectedStatus { - t.Errorf("Expected Run status to be %v but was %v", tc.expectedStatus, condition) - } - if condition.Reason != tc.expectedReason { - t.Errorf("Expected reason to be %q but was %q", tc.expectedReason, condition.Reason) - } - } - } - }) - } -} - -func defaultREADMEHandlerFunc() func(http.ResponseWriter, *http.Request) { - return func(w http.ResponseWriter, r *http.Request) { - _, _ = fmt.Fprint(w, ghContentJSON(defaultTEPReadmeContent)) - } -} - -func noCommentsOnPRHandlerFunc(t *testing.T) func(http.ResponseWriter, *http.Request) { - return func(w http.ResponseWriter, r *http.Request) { - switch r.Method { - case "GET": - _, _ = fmt.Fprint(w, `[]`) - return - case "POST": - _, _ = fmt.Fprint(w, `{"id":1}`) - return - default: - t.Errorf("unexpected method %s", r.Method) - } - } -} - -func constructRunParams(overrides map[string]string, additionalParams map[string]string) []v1beta1.Param { - var params []v1beta1.Param - - for key, defaultValue := range defaultRunParams { - if overrideValue, ok := overrides[key]; ok { - if overrideValue != "" { - params = append(params, v1beta1.Param{ - Name: key, - Value: v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: overrideValue}, - }) - } - } else { - params = append(params, v1beta1.Param{ - Name: key, - Value: v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: defaultValue}, - }) - } - } - - for k, v := range additionalParams { - params = append(params, v1beta1.Param{ - Name: k, - Value: v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: v}, - }) - } - - return params -} - -func ghContentJSON(content string) string { - encContent := base64.StdEncoding.EncodeToString([]byte(content)) - - return fmt.Sprintf(`{ - "type": "file", - "content": "%s", - "encoding": "base64", - "size": 1234, - "name": "SOMEFILE", - "path": "SOMEPATH" - }`, encContent) -} - -func setupFakeGitHub() (*github.Client, *http.ServeMux, func()) { - apiPath := "/api-v3" - - mux := http.NewServeMux() - - handler := http.NewServeMux() - handler.Handle(apiPath+"/", http.StripPrefix(apiPath, mux)) - - server := httptest.NewServer(handler) - - client := github.NewClient(nil) - ghURL, _ := url.Parse(server.URL + apiPath + "/") - client.BaseURL = ghURL - client.UploadURL = ghURL - - return client, mux, server.Close -}