From 6ad7e4a9a0b48622a682a3e4d32e42dd1d1b4027 Mon Sep 17 00:00:00 2001
From: Priya Wadhwa <priyawadhwa@google.com>
Date: Mon, 6 Jan 2020 13:42:09 -0800
Subject: [PATCH] address code review comments

add better error handling and remove duplication in enableOrDisableAddonsInternal
---
 pkg/addons/addons.go | 51 +++++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/pkg/addons/addons.go b/pkg/addons/addons.go
index fa08354646..3af61a0d39 100644
--- a/pkg/addons/addons.go
+++ b/pkg/addons/addons.go
@@ -45,22 +45,22 @@ func Set(name, value, profile string) error {
 
 	// Run any additional validations for this property
 	if err := run(name, value, profile, a.validations); err != nil {
-		return err
+		return errors.Wrap(err, "running validations")
 	}
 
 	// Set the value
 	c, err := config.Load(profile)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "loading profile")
 	}
 
 	if err := a.set(c, name, value); err != nil {
-		return err
+		return errors.Wrap(err, "setting new value of addon")
 	}
 
 	// Run any callbacks for this property
 	if err := run(name, value, profile, a.callbacks); err != nil {
-		return err
+		return errors.Wrap(err, "running callbacks")
 	}
 
 	// Write the value
@@ -175,37 +175,24 @@ func isAddonAlreadySet(addon *assets.Addon, enable bool) (bool, error) {
 func enableOrDisableAddonInternal(addon *assets.Addon, cmd command.Runner, data interface{}, enable bool) error {
 	var err error
 
-	if enable {
-		for _, addon := range addon.Assets {
-			var addonFile assets.CopyableFile
-			if addon.IsTemplate() {
-				addonFile, err = addon.Evaluate(data)
-				if err != nil {
-					return errors.Wrapf(err, "evaluate bundled addon %s asset", addon.GetAssetName())
-				}
+	updateFile := cmd.Copy
+	if !enable {
+		updateFile = cmd.Remove
+	}
 
-			} else {
-				addonFile = addon
-			}
-			if err := cmd.Copy(addonFile); err != nil {
-				return errors.Wrapf(err, "enabling addon %s", addon.AssetName)
+	for _, addon := range addon.Assets {
+		var addonFile assets.CopyableFile
+		if addon.IsTemplate() {
+			addonFile, err = addon.Evaluate(data)
+			if err != nil {
+				return errors.Wrapf(err, "evaluate bundled addon %s asset", addon.GetAssetName())
 			}
+
+		} else {
+			addonFile = addon
 		}
-	} else {
-		for _, addon := range addon.Assets {
-			var addonFile assets.CopyableFile
-			if addon.IsTemplate() {
-				addonFile, err = addon.Evaluate(data)
-				if err != nil {
-					return errors.Wrapf(err, "evaluate bundled addon %s asset", addon.GetAssetName())
-				}
-
-			} else {
-				addonFile = addon
-			}
-			if err := cmd.Remove(addonFile); err != nil {
-				return errors.Wrapf(err, "disabling addon %s", addon.AssetName)
-			}
+		if err := updateFile(addonFile); err != nil {
+			return errors.Wrapf(err, "updating addon %s", addon.AssetName)
 		}
 	}
 	return nil