Skip to content

Commit

Permalink
Add structured logger (#54) (#1467)
Browse files Browse the repository at this point in the history 
This is two changes:

Replacing all usages of SimpleLogger with it's interface SimpleLogging which makes it easier to swap out loggers going forward
Nukes SimpleLogger in favor of StructuredLogger which allows us to better analyze logs in our log management system we choose.
  • Loading branch information
@nishkrishnan
nishkrishnan authored Apr 6, 2021
1 parent 13fba40 commit 7ac8106
Show file tree
Hide file tree
Showing 83 changed files with 888 additions and 599 deletions.
2 changes: 1 addition & 1 deletion cmd/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ type ServerCmd struct {
// Useful for testing to keep the logs clean.
SilenceOutput bool
AtlantisVersion string
Logger *logging.SimpleLogger
Logger logging.SimpleLogging
}

// ServerCreator creates servers.
Expand Down
69 changes: 36 additions & 33 deletions cmd/server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

homedir "github.com/mitchellh/go-homedir"
"github.com/runatlantis/atlantis/server"
"github.com/runatlantis/atlantis/server/logging"
. "github.com/runatlantis/atlantis/testing"
"github.com/spf13/cobra"
"github.com/spf13/viper"
Expand Down Expand Up @@ -111,7 +112,7 @@ func TestExecute_Defaults(t *testing.T) {
GHUserFlag: "user",
GHTokenFlag: "token",
RepoAllowlistFlag: "*",
})
}, t)
err := c.Execute()
Ok(t, err)

Expand Down Expand Up @@ -155,7 +156,7 @@ func TestExecute_Defaults(t *testing.T) {

func TestExecute_Flags(t *testing.T) {
t.Log("Should use all flags that are set.")
c := setup(testFlags)
c := setup(testFlags, t)
err := c.Execute()
Ok(t, err)
for flag, exp := range testFlags {
Expand All @@ -173,7 +174,7 @@ func TestExecute_ConfigFile(t *testing.T) {
defer os.Remove(tmpFile) // nolint: errcheck
c := setup(map[string]interface{}{
ConfigFlag: tmpFile,
})
}, t)
err := c.Execute()
Ok(t, err)
for flag, exp := range testFlags {
Expand All @@ -188,7 +189,7 @@ func TestExecute_EnvironmentVariables(t *testing.T) {
os.Setenv(envKey, fmt.Sprintf("%v", value)) // nolint: errcheck
defer func(key string) { os.Unsetenv(key) }(envKey)
}
c := setup(nil)
c := setup(nil, t)
err := c.Execute()
Ok(t, err)
for flag, exp := range testFlags {
Expand All @@ -200,7 +201,7 @@ func TestExecute_NoConfigFlag(t *testing.T) {
t.Log("If there is no config flag specified Execute should return nil.")
c := setupWithDefaults(map[string]interface{}{
ConfigFlag: "",
})
}, t)
err := c.Execute()
Ok(t, err)
}
Expand All @@ -209,7 +210,7 @@ func TestExecute_ConfigFileExtension(t *testing.T) {
t.Log("If the config file doesn't have an extension then error.")
c := setupWithDefaults(map[string]interface{}{
ConfigFlag: "does-not-exist",
})
}, t)
err := c.Execute()
Equals(t, "invalid config: reading does-not-exist: Unsupported Config Type \"\"", err.Error())
}
Expand All @@ -218,7 +219,7 @@ func TestExecute_ConfigFileMissing(t *testing.T) {
t.Log("If the config file doesn't exist then error.")
c := setupWithDefaults(map[string]interface{}{
ConfigFlag: "does-not-exist.yaml",
})
}, t)
err := c.Execute()
Equals(t, "invalid config: reading does-not-exist.yaml: open does-not-exist.yaml: no such file or directory", err.Error())
}
Expand All @@ -229,7 +230,7 @@ func TestExecute_ConfigFileExists(t *testing.T) {
defer os.Remove(tmpFile) // nolint: errcheck
c := setupWithDefaults(map[string]interface{}{
ConfigFlag: tmpFile,
})
}, t)
err := c.Execute()
Ok(t, err)
}
Expand All @@ -240,7 +241,7 @@ func TestExecute_InvalidConfig(t *testing.T) {
defer os.Remove(tmpFile) // nolint: errcheck
c := setupWithDefaults(map[string]interface{}{
ConfigFlag: tmpFile,
})
}, t)
err := c.Execute()
Assert(t, strings.Contains(err.Error(), "unmarshal errors"), "should be an unmarshal error")
}
Expand All @@ -251,7 +252,7 @@ func TestExecute_RepoAllowlistScheme(t *testing.T) {
GHUserFlag: "user",
GHTokenFlag: "token",
RepoAllowlistFlag: "http://github.com/*",
})
}, t)
err := c.Execute()
Assert(t, err != nil, "should be an error")
Equals(t, "--repo-allowlist cannot contain ://, should be hostnames only", err.Error())
Expand Down Expand Up @@ -280,7 +281,7 @@ func TestExecute_ValidateLogLevel(t *testing.T) {
}
for _, testCase := range cases {
t.Log("Should validate log level when " + testCase.description)
c := setupWithDefaults(testCase.flags)
c := setupWithDefaults(testCase.flags, t)
err := c.Execute()
if testCase.expectError {
Assert(t, err != nil, "should be an error")
Expand All @@ -293,7 +294,7 @@ func TestExecute_ValidateLogLevel(t *testing.T) {
func TestExecute_ValidateCheckoutStrategy(t *testing.T) {
c := setupWithDefaults(map[string]interface{}{
CheckoutStrategyFlag: "invalid",
})
}, t)
err := c.Execute()
ErrEquals(t, "invalid checkout strategy: not one of branch or merge", err)
}
Expand Down Expand Up @@ -335,7 +336,7 @@ func TestExecute_ValidateSSLConfig(t *testing.T) {
}
for _, testCase := range cases {
t.Log("Should validate ssl config when " + testCase.description)
c := setupWithDefaults(testCase.flags)
c := setupWithDefaults(testCase.flags, t)
err := c.Execute()
if testCase.expectError {
Assert(t, err != nil, "should be an error")
Expand Down Expand Up @@ -511,7 +512,7 @@ func TestExecute_ValidateVCSConfig(t *testing.T) {
t.Log("Should validate vcs config when " + testCase.description)
testCase.flags[RepoAllowlistFlag] = "*"

c := setup(testCase.flags)
c := setup(testCase.flags, t)
err := c.Execute()
if testCase.expectError {
Assert(t, err != nil, "should be an error")
Expand All @@ -529,7 +530,7 @@ func TestExecute_ExpandHomeInDataDir(t *testing.T) {
GHTokenFlag: "token",
RepoAllowlistFlag: "*",
DataDirFlag: "~/this/is/a/path",
})
}, t)
err := c.Execute()
Ok(t, err)

Expand All @@ -542,7 +543,7 @@ func TestExecute_RelativeDataDir(t *testing.T) {
t.Log("Should convert relative dir to absolute.")
c := setupWithDefaults(map[string]interface{}{
DataDirFlag: "../",
})
}, t)

// Figure out what ../ should be as an absolute path.
expectedAbsolutePath, err := filepath.Abs("../")
Expand All @@ -559,7 +560,7 @@ func TestExecute_GithubUser(t *testing.T) {
GHUserFlag: "@user",
GHTokenFlag: "token",
RepoAllowlistFlag: "*",
})
}, t)
err := c.Execute()
Ok(t, err)

Expand All @@ -572,7 +573,7 @@ func TestExecute_GithubApp(t *testing.T) {
GHAppKeyFileFlag: "key.pem",
GHAppIDFlag: "1",
RepoAllowlistFlag: "*",
})
}, t)
err := c.Execute()
Ok(t, err)

Expand All @@ -585,7 +586,7 @@ func TestExecute_GitlabUser(t *testing.T) {
GitlabUserFlag: "@user",
GitlabTokenFlag: "token",
RepoAllowlistFlag: "*",
})
}, t)
err := c.Execute()
Ok(t, err)

Expand All @@ -598,7 +599,7 @@ func TestExecute_BitbucketUser(t *testing.T) {
BitbucketUserFlag: "@user",
BitbucketTokenFlag: "token",
RepoAllowlistFlag: "*",
})
}, t)
err := c.Execute()
Ok(t, err)

Expand All @@ -611,7 +612,7 @@ func TestExecute_ADUser(t *testing.T) {
ADUserFlag: "@user",
ADTokenFlag: "token",
RepoAllowlistFlag: "*",
})
}, t)
err := c.Execute()
Ok(t, err)

Expand All @@ -625,7 +626,7 @@ func TestExecute_BitbucketCloudWithWebhookSecret(t *testing.T) {
BitbucketTokenFlag: "token",
RepoAllowlistFlag: "*",
BitbucketWebhookSecretFlag: "my secret",
})
}, t)
err := c.Execute()
ErrEquals(t, "--bitbucket-webhook-secret cannot be specified for Bitbucket Cloud because it is not supported by Bitbucket", err)
}
Expand All @@ -637,15 +638,15 @@ func TestExecute_BitbucketServerBaseURLScheme(t *testing.T) {
BitbucketTokenFlag: "token",
RepoAllowlistFlag: "*",
BitbucketBaseURLFlag: "mydomain.com",
})
}, t)
ErrEquals(t, "--bitbucket-base-url must have http:// or https://, got \"mydomain.com\"", c.Execute())

c = setup(map[string]interface{}{
BitbucketUserFlag: "user",
BitbucketTokenFlag: "token",
RepoAllowlistFlag: "*",
BitbucketBaseURLFlag: "://mydomain.com",
})
}, t)
ErrEquals(t, "error parsing --bitbucket-webhook-secret flag value \"://mydomain.com\": parse \"://mydomain.com\": missing protocol scheme", c.Execute())
}

Expand All @@ -656,7 +657,7 @@ func TestExecute_BitbucketServerBaseURLPort(t *testing.T) {
BitbucketTokenFlag: "token",
RepoAllowlistFlag: "*",
BitbucketBaseURLFlag: "http://mydomain.com:7990",
})
}, t)
Ok(t, c.Execute())
Equals(t, "http://mydomain.com:7990", passedConfig.BitbucketBaseURL)
}
Expand All @@ -669,7 +670,7 @@ func TestExecute_RepoCfgFlags(t *testing.T) {
RepoAllowlistFlag: "github.com",
RepoConfigFlag: "repos.yaml",
RepoConfigJSONFlag: "{}",
})
}, t)
err := c.Execute()
ErrEquals(t, "cannot use --repo-config and --repo-config-json at the same time", err)
}
Expand All @@ -681,7 +682,7 @@ func TestExecute_TFEHostnameOnly(t *testing.T) {
GHTokenFlag: "token",
RepoAllowlistFlag: "github.com",
TFEHostnameFlag: "not-app.terraform.io",
})
}, t)
err := c.Execute()
ErrEquals(t, "if setting --tfe-hostname, must set --tfe-token", err)
}
Expand All @@ -693,7 +694,7 @@ func TestExecute_BothAllowAndWhitelist(t *testing.T) {
GHTokenFlag: "token",
RepoAllowlistFlag: "github.com",
RepoWhitelistFlag: "github.com",
})
}, t)
err := c.Execute()
ErrEquals(t, "both --repo-allowlist and --repo-whitelist cannot be set–use --repo-allowlist", err)
}
Expand All @@ -703,7 +704,7 @@ func TestExecute_AllowAndWhitelist(t *testing.T) {
c := setup(map[string]interface{}{
GHUserFlag: "user",
GHTokenFlag: "token",
})
}, t)
err := c.Execute()
ErrEquals(t, "--repo-allowlist must be set for security purposes", err)
}
Expand All @@ -716,7 +717,7 @@ func TestExecute_BothSilenceAllowAndWhitelistErrors(t *testing.T) {
RepoAllowlistFlag: "*",
SilenceWhitelistErrorsFlag: true,
SilenceAllowlistErrorsFlag: true,
})
}, t)
err := c.Execute()
ErrEquals(t, "both --silence-allowlist-errors and --silence-whitelist-errors cannot be set–use --silence-allowlist-errors", err)
}
Expand All @@ -729,14 +730,14 @@ func TestExecute_RepoWhitelistDeprecation(t *testing.T) {
GHTokenFlag: "token",
RepoWhitelistFlag: "*",
SilenceWhitelistErrorsFlag: true,
})
}, t)
err := c.Execute()
Ok(t, err)
Equals(t, true, passedConfig.SilenceAllowlistErrors)
Equals(t, "*", passedConfig.RepoAllowlist)
}

func setup(flags map[string]interface{}) *cobra.Command {
func setup(flags map[string]interface{}, t *testing.T) *cobra.Command {
vipr := viper.New()
for k, v := range flags {
vipr.Set(k, v)
Expand All @@ -745,11 +746,12 @@ func setup(flags map[string]interface{}) *cobra.Command {
ServerCreator: &ServerCreatorMock{},
Viper: vipr,
SilenceOutput: true,
Logger: logging.NewNoopLogger(t),
}
return c.Init()
}

func setupWithDefaults(flags map[string]interface{}) *cobra.Command {
func setupWithDefaults(flags map[string]interface{}, t *testing.T) *cobra.Command {
vipr := viper.New()
flags[GHUserFlag] = "user"
flags[GHTokenFlag] = "token"
Expand All @@ -762,6 +764,7 @@ func setupWithDefaults(flags map[string]interface{}) *cobra.Command {
ServerCreator: &ServerCreatorMock{},
Viper: vipr,
SilenceOutput: true,
Logger: logging.NewNoopLogger(t),
}
return c.Init()
}
Expand Down
Loading

0 comments on commit 7ac8106

Please sign in to comment.