diff --git a/server/events/markdown_renderer.go b/server/events/markdown_renderer.go index 124bd74dc8..ccbbd6887c 100644 --- a/server/events/markdown_renderer.go +++ b/server/events/markdown_renderer.go @@ -39,7 +39,12 @@ var ( ) // MarkdownRenderer renders responses as markdown. -type MarkdownRenderer struct{} +type MarkdownRenderer struct { + // GitlabSupportsCommonMark is true if the version of GitLab we're + // using supports the CommonMark markdown format. + // If we're not configured with a GitLab client, this will be false. + GitlabSupportsCommonMark bool +} // CommonData is data that all responses have. type CommonData struct { @@ -156,13 +161,18 @@ func (m *MarkdownRenderer) renderProjectResults(results []ProjectResult, common // shouldUseWrappedTmpl returns true if we should use the wrapped markdown // templates that collapse the output to make the comment smaller on initial -// load. +// load. Some VCS providers or versions of VCS providers don't support this +// syntax. func (m *MarkdownRenderer) shouldUseWrappedTmpl(vcsHost models.VCSHostType, output string) bool { // Bitbucket Cloud and Server don't support the folding markdown syntax. if vcsHost == models.BitbucketServer || vcsHost == models.BitbucketCloud { return false } + if vcsHost == models.Gitlab && !m.GitlabSupportsCommonMark { + return false + } + return strings.Count(output, "\n") > maxUnwrappedLines } diff --git a/server/events/markdown_renderer_test.go b/server/events/markdown_renderer_test.go index ddebe6edef..3d8d791e19 100644 --- a/server/events/markdown_renderer_test.go +++ b/server/events/markdown_renderer_test.go @@ -582,56 +582,74 @@ $$$ // VCS hosts during an error. func TestRenderProjectResults_WrappedErr(t *testing.T) { cases := []struct { - VCSHost models.VCSHostType - Output string - ShouldWrap bool + VCSHost models.VCSHostType + GitlabCommonMarkSupport bool + Output string + ShouldWrap bool }{ { - models.Github, - strings.Repeat("line\n", 1), - false, + VCSHost: models.Github, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, }, { - models.Github, - strings.Repeat("line\n", 13), - true, + VCSHost: models.Github, + Output: strings.Repeat("line\n", 13), + ShouldWrap: true, }, { - models.Gitlab, - strings.Repeat("line\n", 1), - false, + VCSHost: models.Gitlab, + GitlabCommonMarkSupport: false, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, }, { - models.Gitlab, - strings.Repeat("line\n", 13), - true, + VCSHost: models.Gitlab, + GitlabCommonMarkSupport: false, + Output: strings.Repeat("line\n", 13), + ShouldWrap: false, }, { - models.BitbucketCloud, - strings.Repeat("line\n", 1), - false, + VCSHost: models.Gitlab, + GitlabCommonMarkSupport: true, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, }, { - models.BitbucketCloud, - strings.Repeat("line\n", 13), - false, + VCSHost: models.Gitlab, + GitlabCommonMarkSupport: true, + Output: strings.Repeat("line\n", 13), + ShouldWrap: true, }, { - models.BitbucketServer, - strings.Repeat("line\n", 1), - false, + VCSHost: models.BitbucketCloud, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, }, { - models.BitbucketServer, - strings.Repeat("line\n", 13), - false, + VCSHost: models.BitbucketCloud, + Output: strings.Repeat("line\n", 13), + ShouldWrap: false, + }, + { + VCSHost: models.BitbucketServer, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, + }, + { + VCSHost: models.BitbucketServer, + Output: strings.Repeat("line\n", 13), + ShouldWrap: false, }, } - mr := events.MarkdownRenderer{} for _, c := range cases { t.Run(fmt.Sprintf("%s_%v", c.VCSHost.String(), c.ShouldWrap), func(t *testing.T) { + mr := events.MarkdownRenderer{ + GitlabSupportsCommonMark: c.GitlabCommonMarkSupport, + } + rendered := mr.Render(events.CommandResult{ ProjectResults: []events.ProjectResult{ { @@ -675,57 +693,74 @@ $$$ // VCS hosts for a single project. func TestRenderProjectResults_WrapSingleProject(t *testing.T) { cases := []struct { - VCSHost models.VCSHostType - Output string - ShouldWrap bool + VCSHost models.VCSHostType + GitlabCommonMarkSupport bool + Output string + ShouldWrap bool }{ { - models.Github, - strings.Repeat("line\n", 1), - false, + VCSHost: models.Github, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, }, { - models.Github, - strings.Repeat("line\n", 13), - true, + VCSHost: models.Github, + Output: strings.Repeat("line\n", 13), + ShouldWrap: true, + }, + { + VCSHost: models.Gitlab, + GitlabCommonMarkSupport: false, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, }, { - models.Gitlab, - strings.Repeat("line\n", 1), - false, + VCSHost: models.Gitlab, + GitlabCommonMarkSupport: false, + Output: strings.Repeat("line\n", 13), + ShouldWrap: false, }, { - models.Gitlab, - strings.Repeat("line\n", 13), - true, + VCSHost: models.Gitlab, + GitlabCommonMarkSupport: true, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, }, { - models.BitbucketCloud, - strings.Repeat("line\n", 1), - false, + VCSHost: models.Gitlab, + GitlabCommonMarkSupport: true, + Output: strings.Repeat("line\n", 13), + ShouldWrap: true, }, { - models.BitbucketCloud, - strings.Repeat("line\n", 13), - false, + VCSHost: models.BitbucketCloud, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, }, { - models.BitbucketServer, - strings.Repeat("line\n", 1), - false, + VCSHost: models.BitbucketCloud, + Output: strings.Repeat("line\n", 13), + ShouldWrap: false, }, { - models.BitbucketServer, - strings.Repeat("line\n", 13), - false, + VCSHost: models.BitbucketServer, + Output: strings.Repeat("line\n", 1), + ShouldWrap: false, + }, + { + VCSHost: models.BitbucketServer, + Output: strings.Repeat("line\n", 13), + ShouldWrap: false, }, } - mr := events.MarkdownRenderer{} for _, c := range cases { for _, cmd := range []events.CommandName{events.PlanCommand, events.ApplyCommand} { t.Run(fmt.Sprintf("%s_%s_%v", c.VCSHost.String(), cmd.String(), c.ShouldWrap), func(t *testing.T) { + mr := events.MarkdownRenderer{ + GitlabSupportsCommonMark: c.GitlabCommonMarkSupport, + } var pr events.ProjectResult switch cmd { case events.PlanCommand: diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index 6563bd4b21..508ae7d2de 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -15,7 +15,13 @@ package vcs import ( "fmt" + "net" "net/url" + "strings" + + "github.com/hashicorp/go-version" + "github.com/pkg/errors" + "github.com/runatlantis/atlantis/server/logging" "github.com/lkysow/go-gitlab" "github.com/runatlantis/atlantis/server/events/models" @@ -23,6 +29,61 @@ import ( type GitlabClient struct { Client *gitlab.Client + // Version is set to the server version. + Version *version.Version +} + +// commonMarkSupported is a version constraint that is true when this version of +// GitLab supports CommonMark, a markdown specification. +// See https://about.gitlab.com/2018/07/22/gitlab-11-1-released/ +var commonMarkSupported = MustConstraint(">=11.1") + +// gitlabClientUnderTest is true if we're running under go test. +var gitlabClientUnderTest = false + +// NewGitlabClient returns a valid GitLab client. +func NewGitlabClient(hostname string, token string, logger *logging.SimpleLogger) (*GitlabClient, error) { + client := &GitlabClient{ + Client: gitlab.NewClient(nil, token), + } + + // If not using gitlab.com we need to set the URL to the API. + if hostname != "gitlab.com" { + u, err := url.Parse(hostname) + if err != nil { + return nil, errors.Wrapf(err, "parsing hostname %q", hostname) + } + + // Warn if this hostname isn't resolvable. The GitLab client + // doesn't give good error messages in this case. + ips, err := net.LookupIP(u.Hostname()) + if err != nil { + logger.Warn("unable to resolve %q: %s", u.Hostname(), err) + } else if len(ips) == 0 { + logger.Warn("found no IPs while resolving %q", u.Hostname()) + } + + scheme := "https" + if u.Scheme != "" { + scheme = u.Scheme + } + apiURL := fmt.Sprintf("%s://%s/api/v4/", scheme, u.Host) + if err := client.Client.SetBaseURL(apiURL); err != nil { + return nil, errors.Wrapf(err, "setting GitLab API URL: %s", apiURL) + } + } + + // Determine which version of GitLab is running. + if !gitlabClientUnderTest { + var err error + client.Version, err = client.GetVersion() + if err != nil { + return nil, err + } + logger.Info("determined GitLab is running version %s", client.Version.String()) + } + + return client, nil } // GetModifiedFiles returns the names of files that were modified in the merge request. @@ -103,3 +164,46 @@ func (g *GitlabClient) GetMergeRequest(repoFullName string, pullNum int) (*gitla mr, _, err := g.Client.MergeRequests.GetMergeRequest(repoFullName, pullNum) return mr, err } + +// GetVersion returns the version of the Gitlab server this client is using. +func (g *GitlabClient) GetVersion() (*version.Version, error) { + req, err := g.Client.NewRequest("GET", "/version", nil, nil) + if err != nil { + return nil, err + } + versionResp := new(gitlab.Version) + _, err = g.Client.Do(req, versionResp) + if err != nil { + return nil, err + } + // We need to strip any "-ee" or similar from the resulting version because go-version + // uses that in its constraints and it breaks the comparison we're trying + // to do for Common Mark. + split := strings.Split(versionResp.Version, "-") + parsedVersion, err := version.NewVersion(split[0]) + if err != nil { + return nil, errors.Wrapf(err, "parsing response to /version: %q", versionResp.Version) + } + return parsedVersion, nil +} + +// SupportsCommonMark returns true if the version of Gitlab this client is +// using supports the CommonMark markdown format. +func (g *GitlabClient) SupportsCommonMark() bool { + // This function is called even if we didn't construct a gitlab client + // so we need to handle that case. + if g == nil { + return false + } + + return commonMarkSupported.Check(g.Version) +} + +// MustConstraint returns a constraint. It panics on error. +func MustConstraint(constraint string) version.Constraints { + c, err := version.NewConstraint(constraint) + if err != nil { + panic(err) + } + return c +} diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go new file mode 100644 index 0000000000..16215a53cc --- /dev/null +++ b/server/events/vcs/gitlab_client_test.go @@ -0,0 +1,81 @@ +package vcs + +import ( + "testing" + + "github.com/hashicorp/go-version" + . "github.com/runatlantis/atlantis/testing" +) + +// Test that the base url gets set properly. +func TestNewGitlabClient_BaseURL(t *testing.T) { + gitlabClientUnderTest = true + defer func() { gitlabClientUnderTest = false }() + cases := []struct { + Hostname string + ExpBaseURL string + }{ + { + "gitlab.com", + "https://gitlab.com/api/v4/", + }, + { + "http://custom.domain", + "http://custom.domain/api/v4/", + }, + { + "https://custom.domain", + "https://custom.domain/api/v4/", + }, + } + + for _, c := range cases { + t.Run(c.Hostname, func(t *testing.T) { + client, err := NewGitlabClient(c.Hostname, "token", nil) + Ok(t, err) + Equals(t, c.ExpBaseURL, client.Client.BaseURL().String()) + }) + } +} + +// This function gets called even if GitlabClient is nil +// so we need to test that. +func TestGitlabClient_SupportsCommonMarkNil(t *testing.T) { + var gl *GitlabClient + Equals(t, false, gl.SupportsCommonMark()) +} + +func TestGitlabClient_SupportsCommonMark(t *testing.T) { + cases := []struct { + version string + exp bool + }{ + { + "11.0", + false, + }, + { + "11.1", + true, + }, + { + "11.2", + true, + }, + { + "12.0", + true, + }, + } + + for _, c := range cases { + t.Run(c.version, func(t *testing.T) { + vers, err := version.NewVersion(c.version) + Ok(t, err) + gl := GitlabClient{ + Version: vers, + } + Equals(t, c.exp, gl.SupportsCommonMark()) + }) + } +} diff --git a/server/server.go b/server/server.go index 488f87cf7e..5128e9ce65 100644 --- a/server/server.go +++ b/server/server.go @@ -21,7 +21,6 @@ import ( "flag" "fmt" "log" - "net" "net/http" "net/url" "os" @@ -32,7 +31,6 @@ import ( "github.com/elazarl/go-bindata-assetfs" "github.com/gorilla/mux" - "github.com/lkysow/go-gitlab" "github.com/pkg/errors" "github.com/runatlantis/atlantis/server/events" "github.com/runatlantis/atlantis/server/events/locking" @@ -155,34 +153,10 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { } if userConfig.GitlabUser != "" { supportedVCSHosts = append(supportedVCSHosts, models.Gitlab) - gitlabClient = &vcs.GitlabClient{ - Client: gitlab.NewClient(nil, userConfig.GitlabToken), - } - // If not using gitlab.com we need to set the URL to the API. - if userConfig.GitlabHostname != "gitlab.com" { - // Check if they've also provided a scheme so we don't prepend it - // again. - scheme := "https" - hostname := userConfig.GitlabHostname - schemeSplit := strings.Split(userConfig.GitlabHostname, "://") - if len(schemeSplit) > 1 { - scheme = schemeSplit[0] - hostname = schemeSplit[1] - } - - // Warn if this hostname isn't resolvable. The GitLab client - // doesn't give good error messages in this case. - ips, err := net.LookupIP(hostname) - if err != nil { - logger.Warn("unable to resolve %q: %s", hostname, err) - } else if len(ips) == 0 { - logger.Warn("found no IPs while resolving %q", hostname) - } - - apiURL := fmt.Sprintf("%s://%s/api/v4/", scheme, hostname) - if err := gitlabClient.Client.SetBaseURL(apiURL); err != nil { - return nil, errors.Wrapf(err, "setting GitLab API URL: %s", apiURL) - } + var err error + gitlabClient, err = vcs.NewGitlabClient(userConfig.GitlabHostname, userConfig.GitlabToken, logger) + if err != nil { + return nil, err } } if userConfig.BitbucketUser != "" { @@ -231,7 +205,9 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { if err != nil && flag.Lookup("test.v") == nil { return nil, errors.Wrap(err, "initializing terraform") } - markdownRenderer := &events.MarkdownRenderer{} + markdownRenderer := &events.MarkdownRenderer{ + GitlabSupportsCommonMark: gitlabClient.SupportsCommonMark(), + } boltdb, err := boltdb.New(userConfig.DataDir) if err != nil { return nil, err