[STK-322] Rollback upgrade deployment #2

faizanahmad055 · 2018-07-17T12:22:13Z

Work in Progress.

…pgrade-deployment

golangci · 2018-07-17T12:22:59Z

internal/pkg/handler/updated-handler.go

+const (
+	configmapUpdateOnChangeAnnotation = "reloader.stakater.com/configmap.update-on-change"
+	// Adding seperate annotation to differentiate between configmap and secret
+	secretUpdateOnChangeAnnotation = "reloader.stakater.com/secret.update-on-change"


G101: Potential hardcoded credentials

golangci · 2018-07-17T12:23:00Z

internal/pkg/handler/updated-handler.go

+		namespace = r.Resource.(*v1.ConfigMap).Namespace
+		name = r.Resource.(*v1.ConfigMap).Name
+		sshData = convertConfigmapToSHA(r.Resource.(*v1.ConfigMap))
+		envName = "_CONFIGMAP"


string _CONFIGMAP has 4 occurrences, make it a constant

golangci · 2018-07-17T12:23:00Z

internal/pkg/handler/updated-handler.go

+		namespace = r.Resource.(*v1.Secret).Namespace
+		name = r.Resource.(*v1.Secret).Name
+		sshData = convertSecretToSHA(r.Resource.(*v1.Secret))
+		envName = "_SECRET"


string _SECRET has 4 occurrences, make it a constant

golangci · 2018-07-17T12:23:00Z

internal/pkg/handler/updated-handler.go

+	}
+
+	if rollingUpgradeType == "deployments" {
+		rollingUpgradeForDeployment(client, r, namespace, name, sshData, envName)


Error return value of rollingUpgradeForDeployment is not checked

golangci · 2018-07-17T12:23:00Z

internal/pkg/handler/updated-handler.go

+	if rollingUpgradeType == "deployments" {
+		rollingUpgradeForDeployment(client, r, namespace, name, sshData, envName)
+	} else if rollingUpgradeType == "daemonsets" {
+		rollingUpgradeForDaemonSets(client, r, namespace, name, sshData, envName)


Error return value of rollingUpgradeForDaemonSets is not checked

golangci · 2018-07-17T12:23:00Z

internal/pkg/handler/updated-handler.go

+	} else if rollingUpgradeType == "daemonsets" {
+		rollingUpgradeForDaemonSets(client, r, namespace, name, sshData, envName)
+	} else if rollingUpgradeType == "statefulSets" {
+		rollingUpgradeForStatefulSets(client, r, namespace, name, sshData, envName)


Error return value of rollingUpgradeForStatefulSets is not checked

golangci · 2018-07-17T12:23:01Z

internal/pkg/handler/updated-handler.go

+
+func generateSHA(data string) string {
+	hasher := sha1.New()
+	io.WriteString(hasher, data)


Error return value of io.WriteString is not checked

stakater-user · 2018-07-17T12:31:57Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-1

hazim1093 · 2018-07-17T13:07:37Z

README.md

@@ -21,10 +21,20 @@ For a `Deployment` called `foo` have a `ConfigMap` called `foo`. Then add this a
 ```yaml
 metadata:
  annotations:
-    reloader.stakater.com/update-on-change: "foo"
+    reloader.stakater.com/configmap.update-on-change: "foo"


@waseem-h is it good to have configmap. as prefix or configmap- ?

hazim1093 · 2018-07-17T13:10:02Z

internal/pkg/handler/updated-handler.go

@@ -1,8 +1,23 @@
 package handler

 import (
+	"bytes"


Is updated-handler a good name for this? 🤔 @faizanahmad055 @waseem-h

golangci · 2018-07-18T11:20:44Z

internal/pkg/controller/controller_test.go

+
+				if updated != sshData {
+					return false
+				} else {


if block ends with a return statement, so drop this else and outdent its block

golangci · 2018-07-18T11:20:44Z

internal/pkg/controller/controller_test.go

+	return false
+}
+
+func getResourceSsh(containers []v1.Container, envar string) string {


func getResourceSsh should be getResourceSSH

golangci · 2018-07-18T11:20:44Z

internal/pkg/controller/controller_test.go

+	secretNamePrefix                  = "testsecret-reloader"
+	letters                           = []rune("abcdefghijklmnopqrstuvwxyz")
+	configmapUpdateOnChangeAnnotation = "reloader.stakater.com/configmap.update-on-change"
+	secretUpdateOnChangeAnnotation    = "reloader.stakater.com/secret.update-on-change"


G101: Potential hardcoded credentials

golangci · 2018-07-18T11:20:44Z

internal/pkg/handler/updated-handler.go

+const (
+	configmapUpdateOnChangeAnnotation = "reloader.stakater.com/configmap.update-on-change"
+	// Adding separate annotation to differentiate between configmap and secret
+	secretUpdateOnChangeAnnotation = "reloader.stakater.com/secret.update-on-change"


G101: Potential hardcoded credentials

golangci · 2018-07-18T11:20:45Z

internal/pkg/controller/controller_test.go

+	time.Sleep(10 * time.Second)
+
+	logrus.Infof("Step 8: Update configmap for Second time")
+	_, updateErr = configmapClient.Update(updateConfigmap(namespace, configmapName, "aurorasolutions.io"))


ineffectual assignment to updateErr

golangci · 2018-07-18T11:20:45Z

internal/pkg/handler/updated-handler.go

+		namespace = r.Resource.(*v1.ConfigMap).Namespace
+		name = r.Resource.(*v1.ConfigMap).Name
+		sshData = helper.ConvertConfigmapToSHA(r.Resource.(*v1.ConfigMap))
+		envName = "_CONFIGMAP"


string _CONFIGMAP has 4 occurrences, make it a constant

golangci · 2018-07-18T11:20:45Z

internal/pkg/handler/updated-handler.go

+		namespace = r.Resource.(*v1.Secret).Namespace
+		name = r.Resource.(*v1.Secret).Name
+		sshData = helper.ConvertSecretToSHA(r.Resource.(*v1.Secret))
+		envName = "_SECRET"


string _SECRET has 4 occurrences, make it a constant

golangci · 2018-07-18T11:20:45Z

internal/pkg/helper/helper.go

+// GenerateSHA generates SHA from string
+func GenerateSHA(data string) string {
+	hasher := sha1.New()
+	io.WriteString(hasher, data)


Error return value of io.WriteString is not checked

stakater-user · 2018-07-18T11:29:17Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-2

golangci · 2018-07-18T14:57:12Z

internal/pkg/handler/updated-handler.go

+	}
+
+	if rollingUpgradeType == "deployments" {
+		rollingUpgradeForDeployment(client, namespace, name, sshData, envName)


Error return value of rollingUpgradeForDeployment is not checked

golangci · 2018-07-18T14:57:12Z

internal/pkg/handler/updated-handler.go

+	if rollingUpgradeType == "deployments" {
+		rollingUpgradeForDeployment(client, namespace, name, sshData, envName)
+	} else if rollingUpgradeType == "daemonsets" {
+		rollingUpgradeForDaemonSets(client, namespace, name, sshData, envName)


Error return value of rollingUpgradeForDaemonSets is not checked

golangci · 2018-07-18T14:57:12Z

internal/pkg/handler/updated-handler.go

+	} else if rollingUpgradeType == "daemonsets" {
+		rollingUpgradeForDaemonSets(client, namespace, name, sshData, envName)
+	} else if rollingUpgradeType == "statefulSets" {
+		rollingUpgradeForStatefulSets(client, namespace, name, sshData, envName)


Error return value of rollingUpgradeForStatefulSets is not checked

stakater-user · 2018-07-18T15:06:00Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-3

waseem-h

Rename handlers to just the verbs. e.g., update.go, create.go

waseem-h · 2018-07-19T11:31:15Z

internal/pkg/handler/updated-handler.go

+)
+
+const (
+	configmapUpdateOnChangeAnnotation = "reloader.stakater.com/configmap.update-on-change"


Export these annotations by pascal casing them.
Keys should be: configmap.reloader.stakater.com/reload and secret.reloader.stakater.com/reload

Make sure to reuse them instead of copy pasting again into tests etc

waseem-h · 2018-07-19T12:38:42Z

internal/pkg/handler/updated-handler.go

@@ -20,11 +32,210 @@ func (r ResourceUpdatedHandler) Handle() error {
 		// process resource based on its type
 		if _, ok := r.Resource.(*v1.ConfigMap); ok {
 			logrus.Infof("Performing 'Updated' action for resource of type 'configmap'")
+			rollingUpgrade(r, "configmaps", "deployments")


Whats the point of checking for resource type here and passing it to rollingUpgrade and then checking for resource type again in that function? Seems pretty redundant

waseem-h · 2018-07-19T12:40:28Z

internal/pkg/handler/updated-handler.go

+		namespace = r.Resource.(*v1.ConfigMap).Namespace
+		name = r.Resource.(*v1.ConfigMap).Name
+		sshData = helper.ConvertConfigmapToSHA(r.Resource.(*v1.ConfigMap))
+		envName = "_CONFIGMAP"


You don't need to write constants again and again for the same resource. Since you know the resource type, better use that to generate the env name

waseem-h · 2018-07-19T12:41:46Z

internal/pkg/handler/updated-handler.go

+	}
+
+	if rollingUpgradeType == "deployments" {
+		rollingUpgradeForDeployment(client, namespace, name, sshData, envName)


rollingUpgradeForDeployment, rollingUpgradeForDaemonSets, rollingUpgradeForStatefulSets are almost identical to each other. Remove redundancy and figure out a way to make it more generic than copy pasting methods and just changing the resource type

Also think of a better name for these functions or just drop the for if you still want to use these functions. But I think they can be combined into 1 function with no duplication logic

waseem-h · 2018-07-19T12:45:01Z

internal/pkg/handler/updated-handler.go

+	logrus.Infof("Generated environment variable: %s", envar)
+
+	for i := range containers {
+		envs := containers[i].Env


refactor logic in this method for readability

waseem-h · 2018-07-19T12:47:19Z

internal/pkg/helper/helper.go

+}
+
+// ConvertConfigmapToSHA generates SHA for configmap data
+func ConvertConfigmapToSHA(cm *v1.ConfigMap) string {


ConvertConfigMapToSHA and ConvertSecretToSHA are identical except for 1 small thing.
Responsibility of converting some string to SHA should be separate and in a separate package. This helper doesn't really make sense since it isn't clear of what it is supposed to do. Create meaningful packages and try to keep things decoupled from kubernetes objects at the bottom level

hazim1093 · 2018-07-19T13:00:23Z

internal/pkg/handler/updated-handler.go

@@ -236,60 +234,8 @@ func updateContainers(containers []v1.Container, annotationValue string, sshData
 			}
 			containers[i].Env = append(containers[i].Env, e)
 			updated = true
-			logrus.Infof("%s environment variable does not found so creating a new one")
+			logrus.Infof("%s environment variable does not found, creating a new env with value %s", envar, sshData)


Change the text to "environment variable not found" or "environment variable does not exist".

Plus we should log the name of the object i.e. deployment daemonset, stateful set etc instead of the SHA value

hazim1093 · 2018-07-19T13:01:13Z

internal/pkg/helper/helper.go

+	}
+	sort.Strings(values)
+	sha := GenerateSHA(strings.Join(values, ";"))
+	logrus.Infof("SHA for configmap data: %s", sha)


I think we dont need to logg the SHA data

hazim1093 · 2018-07-19T13:03:06Z

internal/pkg/handler/updated-handler.go

@@ -211,7 +209,7 @@ func rollingUpgradeForStatefulSets(client kubernetes.Interface, r ResourceUpdate

 func updateContainers(containers []v1.Container, annotationValue string, sshData string, resourceType string) bool {
 	updated := false
-	envar := "STAKATER_" + convertToEnvVarName(annotationValue) + resourceType
+	envar := "STAKATER_" + helper.ConvertToEnvVarName(annotationValue) + resourceType
 	logrus.Infof("Generated environment variable: %s", envar)


Please mention the name of Deployment, Daemonsets or statefulset on which action is being performed

golangci · 2018-07-19T13:05:13Z

internal/pkg/handlerTester/updated-handler_test.go

@@ -0,0 +1,261 @@
+package handlerTester


don't use MixedCaps in package name; handlerTester should be handlertester

golangci · 2018-07-19T13:05:13Z

internal/pkg/helper/testUtils.go

+
+				if updated != shaData {
+					return false
+				} else {


if block ends with a return statement, so drop this else and outdent its block

golangci · 2018-07-19T13:05:13Z

internal/pkg/helper/testUtils.go

+
+				if updated != shaData {
+					return false
+				} else {


if block ends with a return statement, so drop this else and outdent its block

golangci · 2018-07-19T13:05:13Z

internal/pkg/helper/testUtils.go

+
+				if updated != shaData {
+					return false
+				} else {


if block ends with a return statement, so drop this else and outdent its block

golangci · 2018-07-19T13:05:13Z

internal/pkg/helper/testUtils.go

+var (
+	letters                           = []rune("abcdefghijklmnopqrstuvwxyz")
+	configmapUpdateOnChangeAnnotation = "reloader.stakater.com/configmap.update-on-change"
+	secretUpdateOnChangeAnnotation    = "reloader.stakater.com/secret.update-on-change"


G101: Potential hardcoded credentials

golangci · 2018-07-19T13:05:16Z

internal/pkg/handlerTester/updated-handler_test.go

+
+func TestRollingUpgradeForDeploymentWithSecret(t *testing.T) {
+	shaData := helper.ConvertSecretToSHA(helper.GetSecret(namespace, secretName, "dGVzdFVwZGF0ZWRTZWNyZXRFbmNvZGluZ0ZvclJlbG9hZGVy"))
+	handler.RollingUpgradeForDeployment(client, namespace, secretName, shaData, "_SECRET")


Error return value of handler.RollingUpgradeForDeployment is not checked

golangci · 2018-07-19T13:05:16Z

internal/pkg/handlerTester/updated-handler_test.go

+
+func TestRollingUpgradeForDaemonsetWithConfigmap(t *testing.T) {
+	shaData := helper.ConvertConfigmapToSHA(helper.GetConfigmap(namespace, configmapName, "www.facebook.com"))
+	handler.RollingUpgradeForDaemonSets(client, namespace, configmapName, shaData, "_CONFIGMAP")


Error return value of handler.RollingUpgradeForDaemonSets is not checked

golangci · 2018-07-19T13:05:16Z

internal/pkg/handlerTester/updated-handler_test.go

+
+func TestRollingUpgradeForDaemonsetWithSecret(t *testing.T) {
+	shaData := helper.ConvertSecretToSHA(helper.GetSecret(namespace, secretName, "d3d3LmZhY2Vib29rLmNvbQ=="))
+	handler.RollingUpgradeForDaemonSets(client, namespace, secretName, shaData, "_SECRET")


Error return value of handler.RollingUpgradeForDaemonSets is not checked

golangci · 2018-07-19T13:05:16Z

internal/pkg/handlerTester/updated-handler_test.go

+
+func TestRollingUpgradeForStatefulsetWithConfigmap(t *testing.T) {
+	shaData := helper.ConvertConfigmapToSHA(helper.GetConfigmap(namespace, configmapName, "www.twitter.com"))
+	handler.RollingUpgradeForStatefulSets(client, namespace, configmapName, shaData, "_CONFIGMAP")


Error return value of handler.RollingUpgradeForStatefulSets is not checked

golangci · 2018-07-19T13:05:17Z

internal/pkg/handlerTester/updated-handler_test.go

+
+func TestRollingUpgradeForStatefulsetWithSecret(t *testing.T) {
+	shaData := helper.ConvertSecretToSHA(helper.GetSecret(namespace, secretName, "d3d3LnR3aXR0ZXIuY29t"))
+	handler.RollingUpgradeForStatefulSets(client, namespace, secretName, shaData, "_SECRET")


Error return value of handler.RollingUpgradeForStatefulSets is not checked

golangci · 2018-07-19T13:09:48Z

internal/pkg/helper/testUtils.go

+
+				if updated != shaData {
+					return false
+				} else {


if block ends with a return statement, so drop this else and outdent its block

golangci · 2018-07-19T13:09:48Z

internal/pkg/helper/testUtils.go

+
+				if updated != shaData {
+					return false
+				} else {


if block ends with a return statement, so drop this else and outdent its block

hazim1093 · 2018-07-19T13:09:31Z

internal/pkg/handler/updated-handler.go

@@ -20,11 +32,216 @@ func (r ResourceUpdatedHandler) Handle() error {
 		// process resource based on its type
 		if _, ok := r.Resource.(*v1.ConfigMap); ok {
 			logrus.Infof("Performing 'Updated' action for resource of type 'configmap'")


This log should be changed to something like: "Configmap: 'configmapname' updated, performing update action" and then the next log where pods are restarted can say something like this Deployment, Daemonsets or statefulset was reloaded due to change or etc

stakater-user · 2018-07-19T13:10:10Z

@faizanahmad055 Yikes! You better fix it before anyone else finds out! Build 4 has Failed!

golangci · 2018-07-19T13:22:53Z

internal/pkg/helper/testUtils.go

+
+				if updated != shaData {
+					return false
+				} else {


if block ends with a return statement, so drop this else and outdent its block

golangci · 2018-07-19T13:22:53Z

internal/pkg/helper/testUtils.go

+
+				if updated != shaData {
+					return false
+				} else {


if block ends with a return statement, so drop this else and outdent its block

golangci · 2018-07-19T13:22:54Z

internal/pkg/handler/updated-handler.go

+		shaData = helper.ConvertSecretToSHA(r.Resource.(*v1.Secret))
+		envNamePostfix = "_SECRET"
+	} else {
+			logrus.Warnf("Invalid resource: Resource should be 'Secret' or 'Configmap' but found, %v", r.Resource)


File is not goimports-ed

golangci · 2018-07-19T13:22:54Z

internal/pkg/handler/updated-handler.go

+		namespace = r.Resource.(*v1.ConfigMap).Namespace
+		name = r.Resource.(*v1.ConfigMap).Name
+		shaData = helper.ConvertConfigmapToSHA(r.Resource.(*v1.ConfigMap))
+		envNamePostfix = "_CONFIGMAP"


string _CONFIGMAP has 4 occurrences, make it a constant

golangci · 2018-07-19T13:22:54Z

internal/pkg/handler/updated-handler.go

+		namespace = r.Resource.(*v1.Secret).Namespace
+		name = r.Resource.(*v1.Secret).Name
+		shaData = helper.ConvertSecretToSHA(r.Resource.(*v1.Secret))
+		envNamePostfix = "_SECRET"


string _SECRET has 4 occurrences, make it a constant

golangci · 2018-07-19T13:22:54Z

internal/pkg/handler/updated-handler.go

+	}
+
+	if rollingUpgradeType == "deployments" {
+		RollingUpgradeForDeployment(client, namespace, name, shaData, envNamePostfix)


Error return value of RollingUpgradeForDeployment is not checked

golangci · 2018-07-19T13:22:54Z

internal/pkg/handler/updated-handler.go

+	if rollingUpgradeType == "deployments" {
+		RollingUpgradeForDeployment(client, namespace, name, shaData, envNamePostfix)
+	} else if rollingUpgradeType == "daemonsets" {
+		RollingUpgradeForDaemonSets(client, namespace, name, shaData, envNamePostfix)


Error return value of RollingUpgradeForDaemonSets is not checked

golangci · 2018-07-19T13:22:55Z

internal/pkg/handler/updated-handler.go

+	} else if rollingUpgradeType == "daemonsets" {
+		RollingUpgradeForDaemonSets(client, namespace, name, shaData, envNamePostfix)
+	} else if rollingUpgradeType == "statefulSets" {
+		RollingUpgradeForStatefulSets(client, namespace, name, shaData, envNamePostfix)


Error return value of RollingUpgradeForStatefulSets is not checked

stakater-user · 2018-07-19T13:28:27Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-5

golangci · 2018-07-23T10:26:40Z

internal/pkg/controller/controller_test.go

-	_, err = secretClient.Create(initSecret(namespace, secretName))
+	// Creating secret
+	secretName := secretNamePrefix + "-update-" + common.RandSeq(5)
+	data := "dGVzdFNlY3JldEVuY29kaW5nRm9yUmVsb2FkZXI="


string dGVzdFNlY3JldEVuY29kaW5nRm9yUmVsb2FkZXI= has 3 occurrences, make it a constant

golangci · 2018-07-23T10:26:41Z

internal/pkg/controller/controller_test.go


-	// TODO: Add functionality to verify reloader functionality here
+	// Updating Secret
+	data = "dGVzdFVwZGF0ZWRTZWNyZXRFbmNvZGluZ0ZvclJlbG9hZGVy"


string dGVzdFVwZGF0ZWRTZWNyZXRFbmNvZGluZ0ZvclJlbG9hZGVy has 3 occurrences, make it a constant

golangci · 2018-07-23T10:26:41Z

internal/pkg/testutil/kube.go

+	} else if resourceType == ConfigmapResourceType {
+		configmap := GetConfigmap(namespace, resourceName, data)
+		for k, v := range configmap.Data {
+			values = append(values, k+"="+string(v[:]))


unnecessary conversion

golangci · 2018-07-23T10:26:41Z

internal/pkg/controller/controller_test.go

+	}
+
+	// Creating deployment
+	testutil.CreateDeployment(client, configmapName, namespace)


Error return value of testutil.CreateDeployment is not checked

golangci · 2018-07-23T10:26:41Z

internal/pkg/controller/controller_test.go


-	// TODO: Add functionality to verify reloader functionality here
+	// Creating deployment
+	testutil.CreateDeployment(client, configmapName, namespace)


Error return value of testutil.CreateDeployment is not checked

golangci · 2018-07-23T10:26:44Z

internal/pkg/handler/update_test.go

+
+func TestRollingUpgradeForDeploymentWithSecret(t *testing.T) {
+	shaData := testutil.ConvertResourceToSHA(testutil.SecretResourceType, namespace, secretName, "dGVzdFVwZGF0ZWRTZWNyZXRFbmNvZGluZ0ZvclJlbG9hZGVy")
+	RollingUpgradeDeployment(client, namespace, secretName, shaData, common.SecretEnvarPostfix, common.SecretUpdateOnChangeAnnotation)


Error return value of RollingUpgradeDeployment is not checked

golangci · 2018-07-23T10:26:44Z

internal/pkg/handler/update_test.go

+
+func TestRollingUpgradeForDaemonsetWithConfigmap(t *testing.T) {
+	shaData := testutil.ConvertResourceToSHA(testutil.ConfigmapResourceType, namespace, configmapName, "www.facebook.com")
+	RollingUpgradeDaemonSets(client, namespace, configmapName, shaData, common.ConfigmapEnvarPostfix, common.ConfigmapUpdateOnChangeAnnotation)


Error return value of RollingUpgradeDaemonSets is not checked

golangci · 2018-07-23T10:26:44Z

internal/pkg/handler/update_test.go

+
+func TestRollingUpgradeForDaemonsetWithSecret(t *testing.T) {
+	shaData := testutil.ConvertResourceToSHA(testutil.SecretResourceType, namespace, secretName, "d3d3LmZhY2Vib29rLmNvbQ==")
+	RollingUpgradeDaemonSets(client, namespace, secretName, shaData, common.SecretEnvarPostfix, common.SecretUpdateOnChangeAnnotation)


Error return value of RollingUpgradeDaemonSets is not checked

golangci · 2018-07-23T10:26:44Z

internal/pkg/handler/update_test.go

+
+func TestRollingUpgradeForStatefulsetWithConfigmap(t *testing.T) {
+	shaData := testutil.ConvertResourceToSHA(testutil.ConfigmapResourceType, namespace, configmapName, "www.twitter.com")
+	RollingUpgradeStatefulSets(client, namespace, configmapName, shaData, common.ConfigmapEnvarPostfix, common.ConfigmapUpdateOnChangeAnnotation)


Error return value of RollingUpgradeStatefulSets is not checked

golangci · 2018-07-23T10:26:45Z

internal/pkg/handler/update_test.go

+
+func TestRollingUpgradeForStatefulsetWithSecret(t *testing.T) {
+	shaData := testutil.ConvertResourceToSHA(testutil.SecretResourceType, namespace, secretName, "d3d3LnR3aXR0ZXIuY29t")
+	RollingUpgradeStatefulSets(client, namespace, secretName, shaData, common.SecretEnvarPostfix, common.SecretUpdateOnChangeAnnotation)


Error return value of RollingUpgradeStatefulSets is not checked

stakater-user · 2018-07-23T10:39:20Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-6

stakater-user · 2018-07-23T13:45:37Z

@faizanahmad055 Yikes! You better fix it before anyone else finds out! Build 8 has Failed!

stakater-user · 2018-07-23T14:17:26Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-9

stakater-user · 2018-07-23T14:38:21Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-10

waseem-h · 2018-07-24T07:40:52Z

README.md

@@ -21,10 +21,20 @@ For a `Deployment` called `foo` have a `ConfigMap` called `foo`. Then add this a
 ```yaml
 metadata:
  annotations:
-    reloader.stakater.com/update-on-change: "foo"
+    reloader.stakater.com/configmap.update-on-change: "foo"


@faizanahmad055 update the readme to reflect the new annotations

waseem-h · 2018-07-24T07:41:48Z

deployments/kubernetes/chart/reloader/templates/rbac.yaml

+      - list
+      - get
+      - update
+      - patch


do we really need watching for daemonsets, deployments and statefulsets? I don't think all these permissions are needed for these. Please verify what is needed for the resources and restrict the RBAC

waseem-h · 2018-07-24T07:42:53Z

internal/pkg/common/common.go

+)
+
+var (
+	letters = []rune("abcdefghijklmnopqrstuvwxyz")


This shouldn't be a part of common. Its only needed for tests so move it to testutil and its respective method as well

waseem-h · 2018-07-24T07:44:04Z

internal/pkg/common/common.go

+
+const (
+	// ConfigmapUpdateOnChangeAnnotation is an annotation to detect changes in configmaps
+	ConfigmapUpdateOnChangeAnnotation = "configmap.reloader.stakater.com/reload"


Annotations mustn't be in common. They belong to either the controller, or in handlers since they're only used there. The tests can simply use them without importing the common package

waseem-h · 2018-07-24T07:44:31Z

internal/pkg/common/common.go

+	// SecretUpdateOnChangeAnnotation is an annotation to detect changes in secrets
+	SecretUpdateOnChangeAnnotation = "secret.reloader.stakater.com/reload"
+	// ConfigmapEnvarPostfix is a postfix for configmap envVar
+	ConfigmapEnvarPostfix = "_CONFIGMAP"


There's a typo in these. Fix the names and move them to where they belong, instead of common

waseem-h · 2018-07-24T07:59:38Z

internal/pkg/handler/update.go

+	}
+
+	var shaData, oldSHAdata string
+	shaData = getSHAfromData(r.Resource)


I don't like that we're checking for resource type again and again. Make the sequence like this:

Generate config to resource type as mentioned in the comment below

Generate SHA by passing in the data without checking for the resource type again in the method

If both SHA's aren't equal, then call the rollingUpgradeInternal method

waseem-h · 2018-07-24T08:02:31Z

internal/pkg/handler/update_test.go

+var (
+	client        = getClient()
+	namespace     = "test-handler"
+	configmapName = "testconfigmap-handler-update-" + common.RandSeq(5)


Make sure to verify that configmap and secret names can be this long

waseem-h · 2018-07-24T08:06:29Z

internal/pkg/handler/update_test.go

+}
+
+func getClient() *kubernetes.Clientset {
+	newClient, err := kube.GetClient()


I asked you to use fake client where controller isn't created. So use that

waseem-h · 2018-07-24T08:07:14Z

internal/pkg/controller/controller_test.go

 }

-// Creating a Controller to do a rolling upgrade upon updating the configmap or secret
+func getClient() *kubernetes.Clientset {


Why is this duplicated in 2 test cases? Move it to testutil if its needed elsewhere

waseem-h · 2018-07-24T08:08:41Z

internal/pkg/testutil/kube.go

+}
+
+// VerifyDeploymentUpdate verifies whether deployment has been updated with environment variable or not
+func VerifyDeploymentUpdate(client kubernetes.Interface, namespace string, name string, envarPostfix string, shaData string, annotation string) bool {


All these verification methods are identical. Use a func for fetching containers for each please and use 1 method

waseem-h

Please make changes as requested above @faizanahmad055

golangci · 2018-07-24T12:47:47Z

internal/pkg/constants/annotations.go

+	ConfigmapUpdateOnChangeAnnotation = "configmap.reloader.stakater.com/reload"
+	// SecretUpdateOnChangeAnnotation is an annotation to detect changes in secrets
+	SecretUpdateOnChangeAnnotation = "secret.reloader.stakater.com/reload"
+)


File is not goimports-ed

golangci · 2018-07-24T12:47:47Z

internal/pkg/constants/constants.go

+	SecretEnvarPostfix = "_SECRET"
+	// EnvVarPrefix is a Prefix for environment variable
+	EnvVarPrefix = "STAKATER_"
+)


File is not goimports-ed

stakater-user · 2018-07-24T13:04:00Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-11

stakater-user · 2018-07-24T16:22:59Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-12

waseem-h · 2018-07-25T14:15:16Z

internal/pkg/callbacks/rolling_upgrade.go

+	ItemsFunc        ItemsFunc
+	ContainersFunc   ContainersFunc
+	UpdateFunc       UpdateFunc
+	ResourceTypeFunc ResourceTypeFunc


This is not needed. Callback functions are only used where you're not sure what the logic for doing a specific task is going to be. Here you know that the resource type is a literal string and can be passed as deployment etc

waseem-h · 2018-07-25T14:16:39Z

internal/pkg/callbacks/rolling_upgrade.go

+}
+
+// GetDeploymentTypeName returns Deployment resource type
+func GetDeploymentTypeName() string {


Remove these name funcs

stakater-user · 2018-07-26T09:18:30Z

@faizanahmad055 Yikes! You better fix it before anyone else finds out! Build 2 has Failed!

stakater-user · 2018-07-26T09:21:05Z

@faizanahmad055 Yikes! You better fix it before anyone else finds out! Build 1 has Failed!

stakater-user · 2018-07-26T09:39:17Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-3

stakater-user · 2018-07-26T09:58:42Z

@faizanahmad055 Image is available for testing. docker pull stakater/reloader:PR-2-4

Updates from master

faizanahmad055 added 6 commits July 16, 2018 20:03

Add initial rolling upgrade changes

6a5341a

Merge branch 'master' of github.com:stakater/Reloader into rollback-u…

d2ae8ce

…pgrade-deployment

Add seperate annotation for secret

d9379d1

Fix SHA generation

6bc9072

Fix testcase with updated annotation

f05d4ed

Update readme with secret annotation

a3f8f30

golangci reviewed Jul 17, 2018

View reviewed changes

hazim1093 reviewed Jul 17, 2018

View reviewed changes

Add helper class and complete integration test

a317555

golangci reviewed Jul 18, 2018

View reviewed changes

Fix rbac permission issue

f89f59c

golangci reviewed Jul 18, 2018

View reviewed changes

waseem-h suggested changes Jul 19, 2018

View reviewed changes

Add handler testcases

17b6d58

hazim1093 requested changes Jul 19, 2018

View reviewed changes

golangci reviewed Jul 19, 2018

View reviewed changes

Fix helper package error

97b7286

golangci reviewed Jul 19, 2018

View reviewed changes

hazim1093 requested changes Jul 19, 2018

View reviewed changes

Optimize rollingUpgrade method

5befb4d

golangci reviewed Jul 19, 2018

View reviewed changes

Implement PR-2 review comments

81c7b3e

golangci reviewed Jul 23, 2018

View reviewed changes

Implement golangci review comments on PR-2

034d2dc

Added Daemonset and statefulset e2e test

effc40c

Fix controller e2e test cases

c7d4a0a

Remove SHA data log

61a2af1

waseem-h approved these changes Jul 24, 2018

View reviewed changes

waseem-h suggested changes Jul 24, 2018

View reviewed changes

Revamp perform rolling method

5b467b7

golangci reviewed Jul 24, 2018

View reviewed changes

faizanahmad055 added 2 commits July 24, 2018 21:05

Implement PR-2 review comments

2cac0cc

Use fake client for update tests

e11e174

waseem-h suggested changes Jul 25, 2018

View reviewed changes

faizanahmad055 added 3 commits July 26, 2018 12:59

Implement PR-2 review comments

c8c0f98

Merge branch 'master' into rollback-upgrade-deployment

b99994e

Fix manifest and .version

0454d18

waseem-h closed this Jul 26, 2018

faizanahmad055 reopened this Jul 26, 2018

Update fabric8 pipeline library version

dd2b3de

waseem-h approved these changes Jul 26, 2018

View reviewed changes

waseem-h merged commit 0b0679b into master Jul 26, 2018

waseem-h deleted the rollback-upgrade-deployment branch July 26, 2018 11:15

faizanahmad055 pushed a commit that referenced this pull request Jan 5, 2023

Merge pull request #2 from stakater/master

481eeef

Updates from master

Felix-Stakater pushed a commit that referenced this pull request May 12, 2025

Fix lint (#2)

f9561a2

Uh oh!

[STK-322] Rollback upgrade deployment #2

[STK-322] Rollback upgrade deployment #2

Uh oh!

Conversation

faizanahmad055 commented Jul 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stakater-user commented Jul 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazim1093 Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stakater-user commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stakater-user commented Jul 18, 2018

Uh oh!

waseem-h left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazim1093 Jul 17, 2018 •

edited

Loading