Refactor error handling. Add comments.

1. Refactor error handling to a) use custom types for our own errors and b) include a stacktrace with all errors. 2. Update tests to look for specific error types instead of simply checking an error is or isn’t nil. 3. Add comments through out the code.
DevOpsProUK · May 31, 2016 · 723cb19 · 723cb19
1 parent dc2bc31
commit 723cb19
Show file tree

Hide file tree

Showing 22 changed files with 437 additions and 156 deletions.
diff --git a/README.md b/README.md
@@ -245,6 +245,32 @@ cd remote
 go test -v -parallel 128 -run TestToTerraformRemoteConfigArgsNoBackendConfigs
 ```
 
+#### Debug logging
+
+If you set the `DEBUG` environment variable to "true", the stack trace for any error will be printed to stdout.
+
+#### Error handling
+
+In this project, we try to ensure that:
+
+1. Every error has a stacktrace. This makes debugging much easier.
+1. Every error generated by our own code (as opposed to errors from Go built-in functions or errors from 3rd party
+   libraries) has a custom type. This makes error handling more precise, as we can decide to handle different types of
+   errors differently.
+
+To accomplish these two goals, we have created an `errors` package that has several helper methods, such as
+`errors.WithStackTrace(err error)`, which wraps the given `error` in an Error object that contains a stacktrace. Under
+the hood, the `errors` package is using the [go-errors](https://github.com/go-errors/errors) library, but this may
+change in the future, so the rest of the code should not depend on `go-errors` directly.
+
+Here is how the `errors` package should be used:
+
+1. Any time you want to create your own error, create a custom type for it that is publicly accessible, and always wrap
+   it with a call to `errors.WithStackTrace`. That way, any time we call a method defined in the Terragrunt call, you
+   know the error it returns already has a stacktrace and you don't have to wrap it yourself.
+1. Any time you get back an error object from a function built into Go or 3rd party library, immediately wrap it with
+   `errors.WithStackTrace`. This gives us a stacktrace as close to the source as possible.
+
 #### Releasing new versions
 
 To release a new version, just go to the [Releases Page](https://github.com/gruntwork-io/terragrunt/releases) and

diff --git a/cli/cli_app.go b/cli/cli_app.go
@@ -8,6 +8,9 @@ import (
 	"github.com/gruntwork-io/terragrunt/shell"
 )
 
+// Since Terragrunt is just a thin wrapper for Terraform, and we don't want to repeat every single Terraform command
+// in its definition, we don't quite fit into the model of any Go CLI library. Fortunately, urfave/cli allows us to
+// override the whole template used for the Usage Text.
 const CUSTOM_USAGE_TEXT = `DESCRIPTION:
    {{.Name}} - {{.UsageText}}
 
@@ -31,6 +34,7 @@ AUTHOR(S):
    {{end}}
 `
 
+// Create the Terragrunt CLI App
 func CreateTerragruntCli(version string) *cli.App {
 	cli.AppHelpTemplate = CUSTOM_USAGE_TEXT
 
@@ -52,6 +56,8 @@ func CreateTerragruntCli(version string) *cli.App {
 	return app
 }
 
+// The sole action for the app. It forwards all commands directly to Terraform, enforcing a few best practices along
+// the way, such as configuring remote state or acquiring a lock.
 func runApp(cliContext *cli.Context) error {
 	terragruntConfig, err := config.ReadTerragruntConfig()
 	if err != nil {
@@ -65,24 +71,27 @@ func runApp(cliContext *cli.Context) error {
 	}
 
 	if terragruntConfig.DynamoDbLock != nil {
-		return runCommandWithLock(cliContext, terragruntConfig.DynamoDbLock)
+		return runTerraformCommandWithLock(cliContext, terragruntConfig.DynamoDbLock)
 	} else {
-		return runCommand(cliContext)
+		return runTerraformCommand(cliContext)
 	}
 }
 
-func runCommandWithLock(cliContext *cli.Context, lock locks.Lock) error {
+// Run the given Terraform command with the given lock (if the command requires locking)
+func runTerraformCommandWithLock(cliContext *cli.Context, lock locks.Lock) error {
 	switch cliContext.Args().First() {
-	case "apply", "destroy": return locks.WithLock(lock, func() error { return runCommand(cliContext) })
+	case "apply", "destroy": return locks.WithLock(lock, func() error { return runTerraformCommand(cliContext) })
 	case "release-lock": return runReleaseLockCommand(cliContext, lock)
-	default: return runCommand(cliContext)
+	default: return runTerraformCommand(cliContext)
 	}
 }
 
-func runCommand(cliContext *cli.Context) error {
+// Run the given Terraform command
+func runTerraformCommand(cliContext *cli.Context) error {
 	return shell.RunShellCommand("terraform", cliContext.Args()...)
 }
 
+// Release a lock, prompting the user for confirmation first
 func runReleaseLockCommand(cliContext *cli.Context, lock locks.Lock) error {
 	proceed, err := shell.PromptUserForYesNo(fmt.Sprintf("Are you sure you want to release %s?", lock))
 	if err != nil {

diff --git a/config/config.go b/config/config.go
@@ -2,10 +2,10 @@ package config
 
 import (
 	"io/ioutil"
-	"fmt"
 	"github.com/hashicorp/hcl"
 	"github.com/gruntwork-io/terragrunt/dynamodb"
 	"github.com/gruntwork-io/terragrunt/remote"
+	"github.com/gruntwork-io/terragrunt/errors"
 )
 
 const TERRAGRUNT_CONFIG_FILE = ".terragrunt"
@@ -16,29 +16,32 @@ type TerragruntConfig struct {
 	RemoteState  *remote.RemoteState
 }
 
+// Read the Terragrunt config file from its default location
 func ReadTerragruntConfig() (*TerragruntConfig, error) {
 	return parseTerragruntConfigFile(TERRAGRUNT_CONFIG_FILE)
 }
 
+// Parse the Terragrunt config file at the given path
 func parseTerragruntConfigFile(configPath string) (*TerragruntConfig, error) {
 	bytes, err := ioutil.ReadFile(configPath)
 	if err != nil {
-		return nil, fmt.Errorf("Error reading Terragrunt config file %s (did you create one?): %s", configPath, err.Error())
+		return nil, errors.WithStackTraceAndPrefix(err, "Error reading Terragrunt config file %s", configPath)
 	}
 
 	config, err := parseTerragruntConfig(string(bytes))
 	if err != nil {
-		return nil, fmt.Errorf("Error parsing Terragrunt config file %s: %s", configPath, err.Error())
+		return nil, errors.WithStackTraceAndPrefix(err, "Error parsing Terragrunt config file %s", configPath)
 	}
 
 	return config, nil
 }
 
+// Parse the Terragrunt config contained in the given string
 func parseTerragruntConfig(config string) (*TerragruntConfig, error) {
 	terragruntConfig := &TerragruntConfig{}
 
 	if err := hcl.Decode(terragruntConfig, config); err != nil {
-		return nil, err
+		return nil, errors.WithStackTrace(err)
 	}
 
 	if terragruntConfig.DynamoDbLock != nil {

diff --git a/config/config_test.go b/config/config_test.go
@@ -4,6 +4,8 @@ import (
 	"testing"
 	"github.com/gruntwork-io/terragrunt/dynamodb"
 	"github.com/stretchr/testify/assert"
+	"github.com/gruntwork-io/terragrunt/remote"
+	"github.com/gruntwork-io/terragrunt/errors"
 )
 
 func TestParseTerragruntConfigDynamoLockMinimalConfig(t *testing.T) {
@@ -61,7 +63,7 @@ func TestParseTerragruntConfigDynamoLockMissingStateFileId(t *testing.T) {
 	`
 
 	_, err := parseTerragruntConfig(config)
-	assert.NotNil(t, err)
+	assert.True(t, errors.IsError(err, dynamodb.StateFileIdMissing))
 }
 
 func TestParseTerragruntConfigRemoteStateMinimalConfig(t *testing.T) {
@@ -83,6 +85,20 @@ func TestParseTerragruntConfigRemoteStateMinimalConfig(t *testing.T) {
 	assert.Empty(t, terragruntConfig.RemoteState.BackendConfigs)
 }
 
+func TestParseTerragruntConfigRemoteStateMissingBackend(t *testing.T) {
+	t.Parallel()
+
+	config :=
+	`
+	remoteState = {
+	}
+	`
+
+	_, err := parseTerragruntConfig(config)
+	assert.True(t, errors.IsError(err, remote.RemoteBackendMissing))
+}
+
+
 func TestParseTerragruntConfigRemoteStateFullConfig(t *testing.T) {
 	t.Parallel()
 

diff --git a/dynamodb/dynamo_lock.go b/dynamodb/dynamo_lock.go
@@ -6,15 +6,18 @@ import (
 	"github.com/aws/aws-sdk-go/aws/defaults"
 	"github.com/aws/aws-sdk-go/service/dynamodb"
 	"github.com/gruntwork-io/terragrunt/util"
+	"github.com/gruntwork-io/terragrunt/errors"
 )
 
+// A lock that uses AWS's DynamoDB to acquire and release locks
 type DynamoDbLock struct {
 	StateFileId 	string
 	AwsRegion   	string
 	TableName   	string
 	MaxLockRetries	int
 }
 
+// Fill in default configuration values for this lock
 func (dynamoLock *DynamoDbLock) FillDefaults() {
 	if dynamoLock.AwsRegion == "" {
 		dynamoLock.AwsRegion = DEFAULT_AWS_REGION
@@ -29,14 +32,17 @@ func (dynamoLock *DynamoDbLock) FillDefaults() {
 	}
 }
 
+// Validate that this lock is configured correctly
 func (dynamoDbLock *DynamoDbLock) Validate() error {
 	if dynamoDbLock.StateFileId == "" {
-		return fmt.Errorf("The dynamodb.lockType field cannot be empty")
+		return errors.WithStackTrace(StateFileIdMissing)
 	}
 
 	return nil
 }
 
+// Acquire a lock by writing an entry to DynamoDB. If that write fails, it means someone else already has the lock, so
+// retry until they release the lock.
 func (dynamoDbLock DynamoDbLock) AcquireLock() error {
 	util.Logger.Printf("Attempting to acquire lock for state file %s in DynamoDB", dynamoDbLock.StateFileId)
 
@@ -49,9 +55,10 @@ func (dynamoDbLock DynamoDbLock) AcquireLock() error {
 		return err
 	}
 
-	return writeItemToLockTableUntilSuccess(dynamoDbLock.StateFileId, dynamoDbLock.TableName, client, dynamoDbLock.MaxLockRetries)
+	return writeItemToLockTableUntilSuccess(dynamoDbLock.StateFileId, dynamoDbLock.TableName, client, dynamoDbLock.MaxLockRetries, SLEEP_BETWEEN_TABLE_LOCK_ACQUIRE_ATTEMPTS)
 }
 
+// Release a lock by deleting an entry from DynamoDB.
 func (dynamoDbLock DynamoDbLock) ReleaseLock() error {
 	util.Logger.Printf("Attempting to release lock for state file %s in DynamoDB", dynamoDbLock.StateFileId)
 
@@ -68,17 +75,23 @@ func (dynamoDbLock DynamoDbLock) ReleaseLock() error {
 	return nil
 }
 
+// Print a string representation of this lock
 func (dynamoLock DynamoDbLock) String() string {
 	return fmt.Sprintf("DynamoDB lock for state file %s", dynamoLock.StateFileId)
 }
 
+// Create an authenticated client for DynamoDB
 func createDynamoDbClient(awsRegion string) (*dynamodb.DynamoDB, error) {
 	config := defaults.Get().Config.WithRegion(awsRegion)
 
 	_, err := config.Credentials.Get()
 	if err != nil {
-		return nil, fmt.Errorf("Error finding AWS credentials (did you set the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables?): %s", err)
+		return nil, errors.WithStackTraceAndPrefix(err, "Error finding AWS credentials (did you set the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables?)")
 	}
 
 	return dynamodb.New(session.New(), config), nil
-}
+}
+
+var StateFileIdMissing = fmt.Errorf("The dynamodb.stateFileId field cannot be empty")
+
+
diff --git a/dynamodb/dynamo_lock_constants.go b/dynamodb/dynamo_lock_constants.go
@@ -2,6 +2,7 @@ package dynamodb
 
 import "time"
 
+// The names of attributes we use in the DynamoDB lock table
 const ATTR_STATE_FILE_ID = "StateFileId"
 const ATTR_USERNAME = "Username"
 const ATTR_IP = "Ip"

diff --git a/dynamodb/dynamo_lock_item.go b/dynamodb/dynamo_lock_item.go
@@ -7,14 +7,18 @@ import (
 	"github.com/gruntwork-io/terragrunt/util"
 	"github.com/aws/aws-sdk-go/aws"
 	"fmt"
+	"github.com/gruntwork-io/terragrunt/errors"
 )
 
+// Create a DynamoDB key for the given item id
 func createKeyFromItemId(itemId string) map[string]*dynamodb.AttributeValue {
 	return map[string]*dynamodb.AttributeValue {
 		ATTR_STATE_FILE_ID: &dynamodb.AttributeValue{S: aws.String(itemId)},
 	}
 }
 
+// Fetch the metadata for the given item from DynamoDB and display it to stdout. This metadata will contain info about
+// who currently has the lock.
 func displayLockMetadata(itemId string, tableName string, client *dynamodb.DynamoDB) {
 	lockMetadata, err := getLockMetadata(itemId, tableName, client)
 	if err != nil {
@@ -24,6 +28,8 @@ func displayLockMetadata(itemId string, tableName string, client *dynamodb.Dynam
 	}
 }
 
+// Fetch the lock metadata for the given item from DynamoDB. This metadata will contain info about who currently has
+// the lock.
 func getLockMetadata(itemId string, tableName string, client *dynamodb.DynamoDB) (*locks.LockMetadata, error) {
 	output, err := client.GetItem(&dynamodb.GetItemInput{
 		Key: createKeyFromItemId(itemId),
@@ -32,12 +38,13 @@ func getLockMetadata(itemId string, tableName string, client *dynamodb.DynamoDB)
 	})
 
 	if err != nil {
-		return nil, err
+		return nil, errors.WithStackTrace(err)
 	}
 
 	return toLockMetadata(itemId, output.Item)
 }
 
+// Convert the AttributeValue map returned by DynamoDB into a LockMetadata struct
 func toLockMetadata(itemId string, item map[string]*dynamodb.AttributeValue) (*locks.LockMetadata, error) {
 	username, err := getAttribute(item, ATTR_USERNAME)
 	if err != nil {
@@ -56,7 +63,7 @@ func toLockMetadata(itemId string, item map[string]*dynamodb.AttributeValue) (*l
 
 	dateCreated, err := time.Parse(locks.DEFAULT_TIME_FORMAT, dateCreatedStr)
 	if err != nil {
-		return nil, err
+		return nil, errors.WithStackTrace(InvalidDateFormat{Date: dateCreatedStr, UnderlyingErr: err})
 	}
 
 	return &locks.LockMetadata{
@@ -67,15 +74,19 @@ func toLockMetadata(itemId string, item map[string]*dynamodb.AttributeValue) (*l
 	}, nil
 }
 
+// Return the value for the given attribute from the given attribute map, or return an error if that attribute is
+// missing from the map
 func getAttribute(item map[string]*dynamodb.AttributeValue, attribute string) (string, error) {
 	value, exists := item[attribute]
 	if !exists {
-		return "", fmt.Errorf("Could not find attribute %s in item!", attribute)
+		return "", errors.WithStackTrace(AttributeMissing{AttributeName: attribute})
 	}
 
 	return *value.S, nil
 }
 
+// Create a DynamoDB item for the given item id. This item represents a lock and will include metadata about the
+// current user, who is trying to acquire the lock.
 func createItem(itemId string) (map[string]*dynamodb.AttributeValue, error) {
 	lockMetadata, err := locks.CreateLockMetadata(itemId)
 	if err != nil {
@@ -89,3 +100,20 @@ func createItem(itemId string) (map[string]*dynamodb.AttributeValue, error) {
 		ATTR_CREATION_DATE: &dynamodb.AttributeValue{S: aws.String(lockMetadata.DateCreated.String())},
 	}, nil
 }
+
+type AttributeMissing struct {
+	AttributeName string
+}
+
+func (err AttributeMissing) Error() string {
+	return fmt.Sprintf("Could not find attribute %s", err.AttributeName)
+}
+
+type InvalidDateFormat struct {
+	Date 		string
+	UnderlyingErr 	error
+}
+
+func (err InvalidDateFormat) Error() string {
+	return fmt.Sprintf("Unable to parse date %s: %s", err.Date, err.UnderlyingErr.Error())
+}