Repository: keikoproj/upgrade-manager
Pull request #167 (2020-12-18 00:49:59)

Move constants to types so that they can be reused
Can't reuse from current package because it tries to pull dependency on kubernetes api, so build fails with mismatched dependency versions.
Moving constants to types removes dependency and simplifies reuse  outside.

These constants are part of the public API - they describe status of the rollout, so should be easy to consume.

codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167?src=pr&el=h1) Report
> Merging [#167](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167?src=pr&el=desc) (99be40f) into [master](https://codecov.io/gh/keikoproj/upgrade-manager/commit/826594b68cd717deab5433934f0cab346cbaef23?el=desc) (826594b) will **not change** coverage.
> The diff coverage is `23.07%`.

[![Impacted file tree graph](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167/graphs/tree.svg?width=650&height=150&src=pr&token=PbUYkNX6wz)](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #167   +/-   ##
=======================================
  Coverage   68.38%   68.38%           
=======================================
  Files          11       11           
  Lines         933      933           
=======================================
  Hits          638      638           
  Misses        262      262           
  Partials       33       33           
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `68.38% <23.07%> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/v1alpha1/rollingupgrade\_types.go](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167/diff?src=pr&el=tree#diff-YXBpL3YxYWxwaGExL3JvbGxpbmd1cGdyYWRlX3R5cGVzLmdv) | `33.33% <ø> (ø)` | |
| [controllers/rollingupgrade\_controller.go](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcm9sbGluZ3VwZ3JhZGVfY29udHJvbGxlci5nbw==) | `67.91% <23.07%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167?src=pr&el=footer). Last update [826594b...4b39cdb](https://codecov.io/gh/keikoproj/upgrade-manager/pull/167?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

uthark: @shrinandj could you please merge it? I don't have permissions to do it.

Patch:
diff --git a/api/v1alpha1/rollingupgrade_types.go b/api/v1alpha1/rollingupgrade_types.go
index ecf51d8..b77f117 100644
--- a/api/v1alpha1/rollingupgrade_types.go
+++ b/api/v1alpha1/rollingupgrade_types.go
@@ -74,6 +74,15 @@ type RollingUpgradeStatus struct {
 	Conditions []RollingUpgradeCondition `json:"conditions,omitempty"`
 }
 
+const (
+	// StatusRunning marks the CR to be running.
+	StatusRunning = "running"
+	// StatusComplete marks the CR as completed.
+	StatusComplete = "completed"
+	// StatusError marks the CR as errored out.
+	StatusError = "error"
+)
+
 // RollingUpgradeCondition describes the state of the RollingUpgrade
 type RollingUpgradeCondition struct {
 	Type   UpgradeConditionType   `json:"type,omitempty"`
diff --git a/controllers/rollingupgrade_controller.go b/controllers/rollingupgrade_controller.go
index 73284c6..7b32431 100644
--- a/controllers/rollingupgrade_controller.go
+++ b/controllers/rollingupgrade_controller.go
@@ -49,12 +49,6 @@ import (
 )
 
 const (
-	// StatusRunning marks the CR to be running.
-	StatusRunning = "running"
-	// StatusComplete marks the CR as completed.
-	StatusComplete = "completed"
-	// StatusError marks the CR as errored out.
-	StatusError = "error"
 	// JanitorAnnotation is for completed objects.
 	JanitorAnnotation = "janitor/ttl"
 	// ClearCompletedFrequency is the time after which a completed rollingUpgrade object is deleted.
@@ -587,7 +581,7 @@ func (r *RollingUpgradeReconciler) finishExecution(finalStatus string, nodesProc
 	}
 	// end event
 	var level string
-	if finalStatus == StatusComplete {
+	if finalStatus == upgrademgrv1alpha1.StatusComplete {
 		level = EventLevelNormal
 	} else {
 		level = EventLevelWarning
@@ -620,8 +614,8 @@ func (r *RollingUpgradeReconciler) finishExecution(finalStatus string, nodesProc
 func (r *RollingUpgradeReconciler) Process(ctx *context.Context,
 	ruObj *upgrademgrv1alpha1.RollingUpgrade) {
 
-	if ruObj.Status.CurrentStatus == StatusComplete ||
-		ruObj.Status.CurrentStatus == StatusError {
+	if ruObj.Status.CurrentStatus == upgrademgrv1alpha1.StatusComplete ||
+		ruObj.Status.CurrentStatus == upgrademgrv1alpha1.StatusError {
 		r.info(ruObj, "No more processing", "currentStatus", ruObj.Status.CurrentStatus)
 
 		if exists := ruObj.ObjectMeta.Annotations[JanitorAnnotation]; exists == "" {
@@ -643,32 +637,32 @@ func (r *RollingUpgradeReconciler) Process(ctx *context.Context,
 	r.CacheConfig.FlushCache("autoscaling")
 	err := r.populateAsg(ruObj)
 	if err != nil {
-		r.finishExecution(StatusError, 0, ctx, ruObj)
+		r.finishExecution(upgrademgrv1alpha1.StatusError, 0, ctx, ruObj)
 		return
 	}
 
 	//TODO(shri): Ensure that no node is Unschedulable at this time.
 	err = r.populateNodeList(ruObj, r.generatedClient.CoreV1().Nodes())
 	if err != nil {
-		r.finishExecution(StatusError, 0, ctx, ruObj)
+		r.finishExecution(upgrademgrv1alpha1.StatusError, 0, ctx, ruObj)
 		return
 	}
 
 	if err := r.populateLaunchTemplates(); err != nil {
-		r.finishExecution(StatusError, 0, ctx, ruObj)
+		r.finishExecution(upgrademgrv1alpha1.StatusError, 0, ctx, ruObj)
 		return
 	}
 
 	asg, err := r.GetAutoScalingGroup(ruObj.NamespacedName())
 	if err != nil {
 		r.error(ruObj, err, "Unable to load ASG for rolling upgrade")
-		r.finishExecution(StatusError, 0, ctx, ruObj)
+		r.finishExecution(upgrademgrv1alpha1.StatusError, 0, ctx, ruObj)
 		return
 	}
 
 	// Update the CR with some basic info before staring the restack.
 	ruObj.Status.StartTime = time.Now().Format(time.RFC3339)
-	ruObj.Status.CurrentStatus = StatusRunning
+	ruObj.Status.CurrentStatus = upgrademgrv1alpha1.StatusRunning
 	ruObj.Status.NodesProcessed = 0
 	ruObj.Status.TotalNodes = len(asg.Instances)
 
@@ -680,7 +674,7 @@ func (r *RollingUpgradeReconciler) Process(ctx *context.Context,
 	nodesProcessed, err := r.runRestack(ctx, ruObj)
 	if err != nil {
 		r.error(ruObj, err, "Failed to runRestack")
-		r.finishExecution(StatusError, nodesProcessed, ctx, ruObj)
+		r.finishExecution(upgrademgrv1alpha1.StatusError, nodesProcessed, ctx, ruObj)
 		return
 	}
 
@@ -688,11 +682,11 @@ func (r *RollingUpgradeReconciler) Process(ctx *context.Context,
 	r.info(ruObj, "Validating the launch definition of nodes and ASG")
 	if err := r.validateNodesLaunchDefinition(ruObj); err != nil {
 		r.error(ruObj, err, "Launch definition validation failed")
-		r.finishExecution(StatusError, nodesProcessed, ctx, ruObj)
+		r.finishExecution(upgrademgrv1alpha1.StatusError, nodesProcessed, ctx, ruObj)
 		return
 	}
 
-	r.finishExecution(StatusComplete, nodesProcessed, ctx, ruObj)
+	r.finishExecution(upgrademgrv1alpha1.StatusComplete, nodesProcessed, ctx, ruObj)
 }
 
 //Check if ec2Instances and the ASG have same launch config.
@@ -735,9 +729,9 @@ func MarkObjForCleanup(ruObj *upgrademgrv1alpha1.RollingUpgrade) {
 	}
 
 	switch ruObj.Status.CurrentStatus {
-	case StatusComplete:
+	case upgrademgrv1alpha1.StatusComplete:
 		ruObj.ObjectMeta.Annotations[JanitorAnnotation] = ClearCompletedFrequency
-	case StatusError:
+	case upgrademgrv1alpha1.StatusError:
 		ruObj.ObjectMeta.Annotations[JanitorAnnotation] = ClearErrorFrequency
 	}
 }


Test patch:
diff --git a/controllers/rollingupgrade_controller_test.go b/controllers/rollingupgrade_controller_test.go
index e1334ca..1e6cde8 100644
--- a/controllers/rollingupgrade_controller_test.go
+++ b/controllers/rollingupgrade_controller_test.go
@@ -68,7 +68,7 @@ func TestErrorStatusMarkJanitor(t *testing.T) {
 
 	ctx := context.TODO()
 	rcRollingUpgrade.inProcessASGs.Store(someAsg, "processing")
-	rcRollingUpgrade.finishExecution(StatusError, 3, &ctx, instance)
+	rcRollingUpgrade.finishExecution(upgrademgrv1alpha1.StatusError, 3, &ctx, instance)
 	g.Expect(instance.ObjectMeta.Annotations[JanitorAnnotation]).To(gomega.Equal(ClearErrorFrequency))
 	_, exists := rcRollingUpgrade.inProcessASGs.Load(someAsg)
 	g.Expect(exists).To(gomega.BeFalse())
@@ -77,7 +77,7 @@ func TestErrorStatusMarkJanitor(t *testing.T) {
 func TestMarkObjForCleanupCompleted(t *testing.T) {
 	g := gomega.NewGomegaWithT(t)
 	ruObj := &upgrademgrv1alpha1.RollingUpgrade{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
-	ruObj.Status.CurrentStatus = StatusComplete
+	ruObj.Status.CurrentStatus = upgrademgrv1alpha1.StatusComplete
 
 	g.Expect(ruObj.ObjectMeta.Annotations).To(gomega.BeNil())
 	MarkObjForCleanup(ruObj)
@@ -87,7 +87,7 @@ func TestMarkObjForCleanupCompleted(t *testing.T) {
 func TestMarkObjForCleanupError(t *testing.T) {
 	g := gomega.NewGomegaWithT(t)
 	ruObj := &upgrademgrv1alpha1.RollingUpgrade{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
-	ruObj.Status.CurrentStatus = StatusError
+	ruObj.Status.CurrentStatus = upgrademgrv1alpha1.StatusError
 
 	g.Expect(ruObj.ObjectMeta.Annotations).To(gomega.BeNil())
 	MarkObjForCleanup(ruObj)
@@ -937,9 +937,9 @@ func TestFinishExecutionCompleted(t *testing.T) {
 	ctx := context.TODO()
 	mockNodesProcessed := 3
 
-	rcRollingUpgrade.finishExecution(StatusComplete, mockNodesProcessed, &ctx, ruObj)
+	rcRollingUpgrade.finishExecution(upgrademgrv1alpha1.StatusComplete, mockNodesProcessed, &ctx, ruObj)
 
-	g.Expect(ruObj.Status.CurrentStatus).To(gomega.Equal(StatusComplete))
+	g.Expect(ruObj.Status.CurrentStatus).To(gomega.Equal(upgrademgrv1alpha1.StatusComplete))
 	g.Expect(ruObj.Status.NodesProcessed).To(gomega.Equal(mockNodesProcessed))
 	g.Expect(ruObj.Status.EndTime).To(gomega.Not(gomega.BeNil()))
 	g.Expect(ruObj.Status.TotalProcessingTime).To(gomega.Not(gomega.BeNil()))
@@ -972,9 +972,9 @@ func TestFinishExecutionError(t *testing.T) {
 	ctx := context.TODO()
 	mockNodesProcessed := 3
 
-	rcRollingUpgrade.finishExecution(StatusError, mockNodesProcessed, &ctx, ruObj)
+	rcRollingUpgrade.finishExecution(upgrademgrv1alpha1.StatusError, mockNodesProcessed, &ctx, ruObj)
 
-	g.Expect(ruObj.Status.CurrentStatus).To(gomega.Equal(StatusError))
+	g.Expect(ruObj.Status.CurrentStatus).To(gomega.Equal(upgrademgrv1alpha1.StatusError))
 	g.Expect(ruObj.Status.NodesProcessed).To(gomega.Equal(mockNodesProcessed))
 	g.Expect(ruObj.Status.EndTime).To(gomega.Not(gomega.BeNil()))
 	g.Expect(ruObj.Status.TotalProcessingTime).To(gomega.Not(gomega.BeNil()))