Update rolloutblockerror to be consistent with other errors (#397)

Co-authored-by: Panagiotis Nikoloutsopoulos <[email protected]>

pkg/controller/application/application_controller_test.go

@@ -273,6 +273,8 @@ func TestCreateFirstReleaseWithNonExistingRolloutBlockOverride(t *testing.T) {
 	f := newFixture(t)
 	app := newApplication(testAppName)
 	app.Annotations[shipper.RolloutBlocksOverrideAnnotation] = fmt.Sprintf("%s/%s", shippertesting.TestNamespace, "test-non-existing-rolloutblock")
+	invalidRolloutBlockKey := "test-namespace/test-non-existing-rolloutblock"
+	invalidRolloutBlockMessage := shippererrors.NewInvalidRolloutBlockOverrideError(invalidRolloutBlockKey).Error()
 
 	f.objects = append(f.objects, app)
 
@@ -291,7 +293,7 @@ func TestCreateFirstReleaseWithNonExistingRolloutBlockOverride(t *testing.T) {
 			Type:    shipper.ApplicationConditionTypeBlocked,
 			Status:  corev1.ConditionTrue,
 			Reason:  shipper.RolloutBlockReason,
-			Message: "rollout block with name test-namespace/test-non-existing-rolloutblock does not exist",
+			Message: invalidRolloutBlockMessage,
 		},
 		{
 			Type:   shipper.ApplicationConditionTypeReleaseSynced,
@@ -312,7 +314,7 @@ func TestCreateFirstReleaseWithNonExistingRolloutBlockOverride(t *testing.T) {
 
 	f.expectedEvents = []string{
 		fmt.Sprintf(`Normal ApplicationConditionChanged [] -> [Aborting False], [] -> [ValidHistory True], [] -> [ReleaseSynced True], [] -> [RollingOut Unknown no contender release found for application "%s"]`, app.Name),
-		`Normal ApplicationConditionChanged [] -> [Blocked True RolloutsBlocked rollout block with name test-namespace/test-non-existing-rolloutblock does not exist]`,
+		fmt.Sprintf(`Normal ApplicationConditionChanged [] -> [Blocked True RolloutsBlocked %s]`, invalidRolloutBlockMessage),
 	}
 
 	f.run()
@@ -322,6 +324,8 @@ func TestBlockApplication(t *testing.T) {
 	f := newFixture(t)
 
 	rolloutblock := newRolloutBlock(testRolloutBlockName)
+	rolloutBlockKey := fmt.Sprintf("%s/%s", rolloutblock.Namespace, rolloutblock.Name)
+	rolloutBlockMessage := shippererrors.NewRolloutBlockError(rolloutBlockKey).Error()
 	f.objects = append(f.objects, rolloutblock)
 	app := newApplication(testAppName)
 
@@ -340,7 +344,7 @@ func TestBlockApplication(t *testing.T) {
 		{
 			Type:    shipper.ApplicationConditionTypeBlocked,
 			Reason:  shipper.RolloutBlockReason,
-			Message: fmt.Sprintf("rollout block(s) with name(s) %s/%s exist", rolloutblock.Namespace, rolloutblock.Name),
+			Message: rolloutBlockMessage,
 			Status:  corev1.ConditionTrue,
 		},
 		{
@@ -363,7 +367,7 @@ func TestBlockApplication(t *testing.T) {
 	f.expectedEvents = []string{
 		fmt.Sprintf("Warning RolloutBlocked %s/%s", rolloutblock.Namespace, rolloutblock.Name),
 		fmt.Sprintf(`Normal ApplicationConditionChanged [] -> [Aborting False], [] -> [ValidHistory True], [] -> [ReleaseSynced True], [] -> [RollingOut Unknown no contender release found for application "%s"]`, app.Name),
-		fmt.Sprintf(`Normal ApplicationConditionChanged [] -> [Blocked True RolloutsBlocked rollout block(s) with name(s) %s/%s exist]`, rolloutblock.Namespace, rolloutblock.Name),
+		fmt.Sprintf(`Normal ApplicationConditionChanged [] -> [Blocked True RolloutsBlocked %s]`, rolloutBlockMessage),
 	}
 
 	f.run()

pkg/controller/release/release_controller_test.go

@@ -22,6 +22,7 @@ import (
 	shipper "github.com/bookingcom/shipper/pkg/apis/shipper/v1alpha1"
 	shipperfake "github.com/bookingcom/shipper/pkg/client/clientset/versioned/fake"
 	shipperinformers "github.com/bookingcom/shipper/pkg/client/informers/externalversions"
+	shippererrors "github.com/bookingcom/shipper/pkg/errors"
 	shippertesting "github.com/bookingcom/shipper/pkg/testing"
 	apputil "github.com/bookingcom/shipper/pkg/util/application"
 	"github.com/bookingcom/shipper/pkg/util/conditions"
@@ -1789,7 +1790,7 @@ func TestContenderCapacityShouldNotIncreaseWithRolloutBlock(t *testing.T) {
 	)
 
 	expectedContender := contender.release.DeepCopy()
-	rolloutBlockMessage := fmt.Sprintf("rollout block(s) with name(s) %s exist", rolloutBlockKey)
+	rolloutBlockMessage := shippererrors.NewRolloutBlockError(rolloutBlockKey).Error()
 	condBlocked := releaseutil.NewReleaseCondition(
 		shipper.ReleaseConditionTypeBlocked,
 		corev1.ConditionTrue,
@@ -1921,7 +1922,7 @@ func TestContenderTrafficShouldNotIncreaseWithRolloutBlock(t *testing.T) {
 	)
 
 	expectedContender := contender.release.DeepCopy()
-	rolloutBlockMessage := fmt.Sprintf("rollout block(s) with name(s) %s exist", rolloutBlockKey)
+	rolloutBlockMessage := shippererrors.NewRolloutBlockError(rolloutBlockKey).Error()
 	condBlocked := releaseutil.NewReleaseCondition(
 		shipper.ReleaseConditionTypeBlocked,
 		corev1.ConditionTrue,
@@ -2048,7 +2049,7 @@ func TestIncumbentTrafficShouldNotDecreaseWithRolloutBlock(t *testing.T) {
 	)
 
 	expectedContender := contender.release.DeepCopy()
-	rolloutBlockMessage := fmt.Sprintf("rollout block(s) with name(s) %s exist", rolloutBlockKey)
+	rolloutBlockMessage := shippererrors.NewRolloutBlockError(rolloutBlockKey).Error()
 	condBlocked := releaseutil.NewReleaseCondition(
 		shipper.ReleaseConditionTypeBlocked,
 		corev1.ConditionTrue,
@@ -2179,7 +2180,7 @@ func TestIncumbentCapacityShouldNotDecreaseWithRolloutBlock(t *testing.T) {
 	)
 
 	expectedContender := contender.release.DeepCopy()
-	rolloutBlockMessage := fmt.Sprintf("rollout block(s) with name(s) %s exist", rolloutBlockKey)
+	rolloutBlockMessage := shippererrors.NewRolloutBlockError(rolloutBlockKey).Error()
 	condBlocked := releaseutil.NewReleaseCondition(
 		shipper.ReleaseConditionTypeBlocked,
 		corev1.ConditionTrue,

pkg/errors/rolloutblock.go

@@ -9,7 +9,7 @@ type InvalidRolloutBlockOverrideError struct {
 }
 
 func (e InvalidRolloutBlockOverrideError) Error() string {
-	return fmt.Sprintf("rollout block with name %s does not exist",
+	return fmt.Sprintf("rollout block with name %q does not exist",
 		e.RolloutBlockName)
 }
 
@@ -21,17 +21,19 @@ func NewInvalidRolloutBlockOverrideError(invalidRolloutBlockName string) Invalid
 	return InvalidRolloutBlockOverrideError{invalidRolloutBlockName}
 }
 
-type RolloutBlockError string
+type RolloutBlockError struct {
+	RolloutBlockName string
+}
 
 func (e RolloutBlockError) Error() string {
-	return string(e)
+	return fmt.Sprintf("rollout block(s) with name(s) %q exist",
+		e.RolloutBlockName)
 }
 
 func (e RolloutBlockError) ShouldRetry() bool {
 	return false
 }
 
-func NewRolloutBlockError(invalidRolloutBlockName string) RolloutBlockError {
-	return RolloutBlockError(fmt.Sprintf("rollout block(s) with name(s) %s exist",
-		invalidRolloutBlockName))
+func NewRolloutBlockError(rolloutBlockName string) RolloutBlockError {
+	return RolloutBlockError{rolloutBlockName}
 }