diff --git a/cmd/pullrequest-init/main.go b/cmd/pullrequest-init/main.go index 20e4f17e7c4..db49450ec64 100644 --- a/cmd/pullrequest-init/main.go +++ b/cmd/pullrequest-init/main.go @@ -26,11 +26,12 @@ import ( ) var ( - prURL = flag.String("url", "", "The url of the pull request to initialize.") - path = flag.String("path", "", "Path of directory under which PR will be copied") - mode = flag.String("mode", "download", "Whether to operate in download or upload mode") - provider = flag.String("provider", "", "The SCM provider to use. Optional") - skipTLSVerify = flag.Bool("insecure-skip-tls-verify", false, "Enable skipping TLS certificate verification in the git client. Defaults to false") + prURL = flag.String("url", "", "The url of the pull request to initialize.") + path = flag.String("path", "", "Path of directory under which PR will be copied") + mode = flag.String("mode", "download", "Whether to operate in download or upload mode") + provider = flag.String("provider", "", "The SCM provider to use. Optional") + skipTLSVerify = flag.Bool("insecure-skip-tls-verify", false, "Enable skipping TLS certificate verification in the git client. Defaults to false") + disableStrictJSONComments = flag.Bool("disable-strict-json-comments", false, "Disable strict json parsing for comment files with .json extension") ) func main() { @@ -65,7 +66,7 @@ func main() { case "upload": logger.Info("RUNNING UPLOAD!") - r, err := pullrequest.FromDisk(*path) + r, err := pullrequest.FromDisk(*path, *disableStrictJSONComments) if err != nil { logger.Fatal(err) } diff --git a/docs/resources.md b/docs/resources.md index aa3e3043822..7e97bd7469a 100644 --- a/docs/resources.md +++ b/docs/resources.md @@ -506,7 +506,10 @@ References (head and base) describe Git references. They are represented as a set of json files. Comments describe a pull request comment. They are represented as a set of json -files. +files. Add a file or modify the `Body` field in an existing json comment file to +interact with the PR. Files with json extension will be parsed as such. +The content of any comments file(s) with other/no extensions will be treated as +body field of the comment. Other pull request information can be found in `pr.json`. This is a read-only resource. Users should use other subresources (labels, comments, etc) to diff --git a/pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource.go b/pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource.go index 7a045cf0c4d..b8f6dfecae0 100644 --- a/pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource.go +++ b/pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource.go @@ -49,8 +49,9 @@ type Resource struct { // Secrets holds a struct to indicate a field name and corresponding secret name to populate it. Secrets []resourcev1alpha1.SecretParam `json:"secrets"` - PRImage string `json:"-"` - InsecureSkipTLSVerify bool `json:"insecure-skip-tls-verify"` + PRImage string `json:"-"` + InsecureSkipTLSVerify bool `json:"insecure-skip-tls-verify"` + DisableStrictJSONComments bool `json:"disable-strict-json-comments"` } // NewResource create a new git resource to pass to a Task @@ -59,11 +60,12 @@ func NewResource(name, prImage string, r *resourcev1alpha1.PipelineResource) (*R return nil, fmt.Errorf("cannot create a PR resource from a %s Pipeline Resource", r.Spec.Type) } prResource := Resource{ - Name: name, - Type: r.Spec.Type, - Secrets: r.Spec.SecretParams, - PRImage: prImage, - InsecureSkipTLSVerify: false, + Name: name, + Type: r.Spec.Type, + Secrets: r.Spec.SecretParams, + PRImage: prImage, + InsecureSkipTLSVerify: false, + DisableStrictJSONComments: false, } for _, param := range r.Spec.Params { switch { @@ -77,6 +79,12 @@ func NewResource(name, prImage string, r *resourcev1alpha1.PipelineResource) (*R return nil, fmt.Errorf("error occurred converting %q to boolean in Pipeline Resource %s", param.Value, r.Name) } prResource.InsecureSkipTLSVerify = verify + case strings.EqualFold(param.Name, "disable-strict-json-comments"): + strict, err := strconv.ParseBool(param.Value) + if err != nil { + return nil, fmt.Errorf("error occurred converting %q to boolean in Pipeline Resource %s", param.Value, r.Name) + } + prResource.DisableStrictJSONComments = strict } } @@ -101,11 +109,12 @@ func (s *Resource) GetURL() string { // Replacements is used for template replacement on a PullRequestResource inside of a Taskrun. func (s *Resource) Replacements() map[string]string { return map[string]string{ - "name": s.Name, - "type": s.Type, - "url": s.URL, - "provider": s.Provider, - "insecure-skip-tls-verify": strconv.FormatBool(s.InsecureSkipTLSVerify), + "name": s.Name, + "type": s.Type, + "url": s.URL, + "provider": s.Provider, + "insecure-skip-tls-verify": strconv.FormatBool(s.InsecureSkipTLSVerify), + "disable-strict-json-comments": strconv.FormatBool(s.DisableStrictJSONComments), } } @@ -131,6 +140,9 @@ func (s *Resource) getSteps(mode string, sourcePath string) []pipelinev1beta1.St if s.InsecureSkipTLSVerify { args = append(args, "-insecure-skip-tls-verify=true") } + if s.DisableStrictJSONComments { + args = append(args, "-disable-strict-json-comments=true") + } evs := []corev1.EnvVar{} for _, sec := range s.Secrets { diff --git a/pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource_test.go b/pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource_test.go index 1250bf60fd5..4a699720fc9 100644 --- a/pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource_test.go +++ b/pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource_test.go @@ -37,6 +37,7 @@ func TestPullRequest_NewResource(t *testing.T) { tb.PipelineResourceSpecParam("url", url), tb.PipelineResourceSpecParam("provider", "github"), tb.PipelineResourceSpecSecretParam("authToken", "test-secret-key", "test-secret-name"), + tb.PipelineResourceSpecParam("disable-strict-json-comments", "true"), )) got, err := pullrequest.NewResource("test-resource", "override-with-pr:latest", pr) if err != nil { @@ -44,13 +45,14 @@ func TestPullRequest_NewResource(t *testing.T) { } want := &pullrequest.Resource{ - Name: "test-resource", - Type: resourcev1alpha1.PipelineResourceTypePullRequest, - URL: url, - Provider: "github", - Secrets: pr.Spec.SecretParams, - PRImage: "override-with-pr:latest", - InsecureSkipTLSVerify: false, + Name: "test-resource", + Type: resourcev1alpha1.PipelineResourceTypePullRequest, + URL: url, + Provider: "github", + Secrets: pr.Spec.SecretParams, + PRImage: "override-with-pr:latest", + InsecureSkipTLSVerify: false, + DisableStrictJSONComments: true, } if d := cmp.Diff(want, got); d != "" { t.Error(diff.PrintWantGot(d)) @@ -133,6 +135,21 @@ func containerTestCases(mode string) []testcase { Args: []string{"-url", "https://example.com", "-path", workspace, "-mode", mode, "-insecure-skip-tls-verify=true"}, Env: []corev1.EnvVar{}, }}}, + }, { + in: &pullrequest.Resource{ + Name: "strict-json-comments", + URL: "https://example.com", + PRImage: "override-with-pr:latest", + DisableStrictJSONComments: true, + }, + out: []v1beta1.Step{{Container: corev1.Container{ + Name: "pr-source-strict-json-comments-78c5n", + Image: "override-with-pr:latest", + WorkingDir: pipeline.WorkspaceDir, + Command: []string{"/ko-app/pullrequest-init"}, + Args: []string{"-url", "https://example.com", "-path", workspace, "-mode", mode, "-disable-strict-json-comments=true"}, + Env: []corev1.EnvVar{}, + }}}, }} } diff --git a/pkg/pullrequest/api_test.go b/pkg/pullrequest/api_test.go index 828691569f2..91e5d9945f2 100644 --- a/pkg/pullrequest/api_test.go +++ b/pkg/pullrequest/api_test.go @@ -135,7 +135,7 @@ func TestUploadFromDisk(t *testing.T) { t.Fatal(err) } - r, err = FromDisk(dir) + r, err = FromDisk(dir, false) if err != nil { t.Fatal(err) } diff --git a/pkg/pullrequest/disk.go b/pkg/pullrequest/disk.go index 9de471b8132..e453e0c3d39 100644 --- a/pkg/pullrequest/disk.go +++ b/pkg/pullrequest/disk.go @@ -19,11 +19,13 @@ package pullrequest import ( "encoding/gob" "encoding/json" + "fmt" "io/ioutil" "net/url" "os" "path/filepath" "strconv" + "strings" "github.com/jenkins-x/go-scm/scm" ) @@ -160,7 +162,7 @@ func toDisk(path string, r interface{}, perm os.FileMode) error { } // FromDisk outputs a PullRequest object from an on-disk representation at the specified path. -func FromDisk(path string) (*Resource, error) { +func FromDisk(path string, disableJSONComments bool) (*Resource, error) { r := &Resource{ PR: &scm.PullRequest{}, Manifests: make(map[string]Manifest), @@ -182,7 +184,7 @@ func FromDisk(path string) (*Resource, error) { } commentsPath := filepath.Join(path, "comments") - r.Comments, manifest, err = commentsFromDisk(commentsPath) + r.Comments, manifest, err = commentsFromDisk(commentsPath, disableJSONComments) if err != nil { return nil, err } @@ -246,7 +248,7 @@ func manifestFromDisk(path string) (Manifest, error) { return m, nil } -func commentsFromDisk(path string) ([]*scm.Comment, Manifest, error) { +func commentsFromDisk(path string, disableStrictJSON bool) ([]*scm.Comment, Manifest, error) { fis, err := ioutil.ReadDir(path) if isNotExistError(err) { return nil, nil, nil @@ -263,9 +265,16 @@ func commentsFromDisk(path string) ([]*scm.Comment, Manifest, error) { if err != nil { return nil, nil, err } - comment := scm.Comment{} - if err := json.Unmarshal(b, &comment); err != nil { - // The comment might be plain text. + var comment scm.Comment + if strings.EqualFold(filepath.Ext(fi.Name()), ".json") { + if err := json.Unmarshal(b, &comment); err != nil { + if disableStrictJSON { + comment.Body = string(b) + } else { + return nil, nil, fmt.Errorf("error parsing comment file %q: %w", fi.Name(), err) + } + } + } else { comment.Body = string(b) } comments = append(comments, &comment) diff --git a/pkg/pullrequest/disk_test.go b/pkg/pullrequest/disk_test.go index f93fe5eeaf2..c5046144949 100644 --- a/pkg/pullrequest/disk_test.go +++ b/pkg/pullrequest/disk_test.go @@ -232,7 +232,7 @@ func TestFromDiskWithoutComments(t *testing.T) { writeFile(filepath.Join(d, "base.json"), &base) writeFile(filepath.Join(d, "head.json"), &head) - rsrc, err := FromDisk(d) + rsrc, err := FromDisk(d, false) if err != nil { t.Fatal(err) } @@ -341,7 +341,7 @@ func TestFromDisk(t *testing.T) { t.Fatal(err) } - rsrc, err := FromDisk(d) + rsrc, err := FromDisk(d, false) if err != nil { t.Fatal(err) } @@ -420,6 +420,131 @@ func readAndUnmarshal(t *testing.T, p string, v interface{}) { } } +func TestCommentsFromDisk(t *testing.T) { + tests := []struct { + name string + files map[string][]byte + disableStrictJSON bool + expectErr bool + expectedComments []*scm.Comment + }{ + { + name: "comments from .json and plain text files", + files: map[string][]byte{ + "comment_foo.json": []byte(` +{ + "Body": "foo", + "Author": { + "Login": "author" + } +}`), + "comment_bar": []byte("bar"), + }, + expectedComments: []*scm.Comment{ + { + Body: "bar", + }, + { + Body: "foo", + Author: scm.User{Login: "author"}, + }, + }, + }, + { + name: "any non-json extension is considered text", + files: map[string][]byte{ + "comment.ANY": []byte("comment_any"), + }, + expectedComments: []*scm.Comment{ + { + Body: "comment_any", + }, + }, + }, + { + name: "invalid content in .json file", + files: map[string][]byte{ + "comment.json": []byte("invalid_json"), + }, + expectErr: true, + }, + { + name: "JSON extension is case-insensitive", + files: map[string][]byte{ + "comment.JSON": []byte("invalid_json"), + }, + expectErr: true, + }, + { + name: "JSON extension parsing can be disabled using `disable-json-comments`", + files: map[string][]byte{ + "comment.JSON": []byte("comment"), + }, + disableStrictJSON: true, + expectedComments: []*scm.Comment{ + { + Body: "comment", + }, + }, + }, + { + name: "valid JSON files are parsed as such even with `disable-json-comments`", + files: map[string][]byte{ + "comment_foo.json": []byte(` +{ + "Body": "foo", + "Author": { + "Login": "author" + } +}`), + }, + disableStrictJSON: true, + expectedComments: []*scm.Comment{ + { + Body: "foo", + Author: scm.User{Login: "author"}, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d, err := ioutil.TempDir("", "") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(d) + if err := os.MkdirAll(filepath.Join(d, "comments"), 0750); err != nil { + t.Fatal(err) + } + fileNames := []string{} + for fileName, contents := range tt.files { + fileNames = append(fileNames, fileName) + if err := ioutil.WriteFile(filepath.Join(d, "comments", fileName), contents, 0700); err != nil { + t.Fatal(err) + } + } + writeManifest(t, fileNames, filepath.Join(d, "comments", manifestPath)) + + comments, _, err := commentsFromDisk(filepath.Join(d, "comments"), tt.disableStrictJSON) + if err == nil && tt.expectErr { + t.Fatal("expected error, got nil") + } + if err != nil && !tt.expectErr { + t.Fatalf("unexpected error: %v", err) + } + if len(comments) != len(tt.expectedComments) { + t.Fatalf("Comments length does not match expected length. expected %d, got %d: %+v", len(tt.expectedComments), len(comments), comments) + } + for i, c := range tt.expectedComments { + if d := cmp.Diff(c, comments[i]); d != "" { + t.Errorf("Get comments: %s", diff.PrintWantGot(d)) + } + } + }) + } +} + func TestLabelsToDisk(t *testing.T) { type args struct { labels []*scm.Label @@ -682,7 +807,7 @@ func TestFromDiskPRShaWithNullHeadAndBase(t *testing.T) { writeFile(filepath.Join(d, "head.json"), &head) writeFile(filepath.Join(d, "pr.json"), &pr) - rsrc, err := FromDisk(d) + rsrc, err := FromDisk(d, false) if err != nil { t.Fatal(err) }