From 1263ccb4633efbf92fca5b875f24704cf22f4681 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Thu, 29 Nov 2018 15:19:21 -0600 Subject: [PATCH 1/2] Move gitlab client setup into constructor. --- server/events/vcs/gitlab_client.go | 40 +++++++++++++++++++++++++ server/events/vcs/gitlab_client_test.go | 35 ++++++++++++++++++++++ server/server.go | 34 +++------------------ 3 files changed, 79 insertions(+), 30 deletions(-) create mode 100644 server/events/vcs/gitlab_client_test.go diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index 6563bd4b21..314fa720e9 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -15,7 +15,11 @@ package vcs import ( "fmt" + "github.com/pkg/errors" + "github.com/runatlantis/atlantis/server/logging" + "net" "net/url" + "strings" "github.com/lkysow/go-gitlab" "github.com/runatlantis/atlantis/server/events/models" @@ -25,6 +29,42 @@ type GitlabClient struct { Client *gitlab.Client } +// 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" { + // Check if they've also provided a scheme so we don't prepend it + // again. + scheme := "https" + hostname := hostname + schemeSplit := strings.Split(hostname, "://") + 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 := client.Client.SetBaseURL(apiURL); err != nil { + return nil, errors.Wrapf(err, "setting GitLab API URL: %s", apiURL) + } + } + + return client, nil +} + // GetModifiedFiles returns the names of files that were modified in the merge request. // The names include the path to the file from the repo root, ex. parent/child/file.txt. func (g *GitlabClient) GetModifiedFiles(repo models.Repo, pull models.PullRequest) ([]string, error) { diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go new file mode 100644 index 0000000000..8b42820bb9 --- /dev/null +++ b/server/events/vcs/gitlab_client_test.go @@ -0,0 +1,35 @@ +package vcs + +import ( + . "github.com/runatlantis/atlantis/testing" + "testing" +) + +// Test that the base url gets set properly. +func TestNewGitlabClient_BaseURL(t *testing.T) { + 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()) + }) + } +} diff --git a/server/server.go b/server/server.go index 488f87cf7e..2ae23a1fd1 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 != "" { From ba04564e2a15652c03055f706abd917d3936aa71 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Thu, 29 Nov 2018 17:02:43 -0600 Subject: [PATCH 2/2] Disable markdown folding on gitlab < 11.1. On startup, make a call to the /version endpoint of Gitlab to get which version we're running. Then use that version to determine whether we should use the markdown folding syntax where the code is expandable. Versions of Gitlab < 11.1 don't support the "Common Mark" markdown format. Fixes https://github.com/runatlantis/atlantis/issues/315 --- server/events/markdown_renderer.go | 14 ++- server/events/markdown_renderer_test.go | 147 +++++++++++++++--------- server/events/vcs/gitlab_client.go | 92 ++++++++++++--- server/events/vcs/gitlab_client_test.go | 48 +++++++- server/server.go | 4 +- 5 files changed, 231 insertions(+), 74 deletions(-) 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 314fa720e9..508ae7d2de 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -15,20 +15,32 @@ package vcs import ( "fmt" - "github.com/pkg/errors" - "github.com/runatlantis/atlantis/server/logging" "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" ) 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{ @@ -37,31 +49,40 @@ func NewGitlabClient(hostname string, token string, logger *logging.SimpleLogger // If not using gitlab.com we need to set the URL to the API. if hostname != "gitlab.com" { - // Check if they've also provided a scheme so we don't prepend it - // again. - scheme := "https" - hostname := hostname - schemeSplit := strings.Split(hostname, "://") - if len(schemeSplit) > 1 { - scheme = schemeSplit[0] - hostname = schemeSplit[1] + 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(hostname) + ips, err := net.LookupIP(u.Hostname()) if err != nil { - logger.Warn("unable to resolve %q: %s", hostname, err) + logger.Warn("unable to resolve %q: %s", u.Hostname(), err) } else if len(ips) == 0 { - logger.Warn("found no IPs while resolving %q", hostname) + logger.Warn("found no IPs while resolving %q", u.Hostname()) } - apiURL := fmt.Sprintf("%s://%s/api/v4/", scheme, 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 } @@ -143,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 index 8b42820bb9..16215a53cc 100644 --- a/server/events/vcs/gitlab_client_test.go +++ b/server/events/vcs/gitlab_client_test.go @@ -1,12 +1,16 @@ package vcs import ( - . "github.com/runatlantis/atlantis/testing" "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 @@ -33,3 +37,45 @@ func TestNewGitlabClient_BaseURL(t *testing.T) { }) } } + +// 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 2ae23a1fd1..5128e9ce65 100644 --- a/server/server.go +++ b/server/server.go @@ -205,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