Invert and test CheckRestart merge logic

hashicorp · Jan 9, 2018 · 84a1013 · 84a1013
1 parent c341626
commit 84a1013
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 8 deletions.
diff --git a/api/tasks.go b/api/tasks.go
@@ -128,15 +128,15 @@ func (c *CheckRestart) Merge(o *CheckRestart) *CheckRestart {
 		return nc
 	}
 
-	if nc.Limit == 0 {
+	if o.Limit > 0 {
 		nc.Limit = o.Limit
 	}
 
-	if nc.Grace == nil {
+	if o.Grace != nil {
 		nc.Grace = o.Grace
 	}
 
-	if !nc.IgnoreWarnings {
+	if o.IgnoreWarnings {
 		nc.IgnoreWarnings = o.IgnoreWarnings
 	}
 
@@ -185,13 +185,11 @@ func (s *Service) Canonicalize(t *Task, tg *TaskGroup, job *Job) {
 		s.AddressMode = "auto"
 	}
 
-	s.CheckRestart.Canonicalize()
-
 	// Canonicallize CheckRestart on Checks and merge Service.CheckRestart
 	// into each check.
-	for i, c := range s.Checks {
-		s.Checks[i].CheckRestart = c.CheckRestart.Merge(s.CheckRestart)
-		c.CheckRestart.Canonicalize()
+	for i, check := range s.Checks {
+		s.Checks[i].CheckRestart = s.CheckRestart.Merge(check.CheckRestart)
+		s.Checks[i].CheckRestart.Canonicalize()
 	}
 }
 

diff --git a/api/tasks_test.go b/api/tasks_test.go
@@ -3,6 +3,7 @@ package api
 import (
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/nomad/helper"
 	"github.com/stretchr/testify/assert"
@@ -266,3 +267,51 @@ func TestTaskGroup_Canonicalize_Update(t *testing.T) {
 	tg.Canonicalize(job)
 	assert.Nil(t, tg.Update)
 }
+
+// TestService_CheckRestart asserts Service.CheckRestart settings are properly
+// inherited by Checks.
+func TestService_CheckRestart(t *testing.T) {
+	job := &Job{Name: helper.StringToPtr("job")}
+	tg := &TaskGroup{Name: helper.StringToPtr("group")}
+	task := &Task{Name: "task"}
+	service := &Service{
+		CheckRestart: &CheckRestart{
+			Limit:          11,
+			Grace:          helper.TimeToPtr(11 * time.Second),
+			IgnoreWarnings: true,
+		},
+		Checks: []ServiceCheck{
+			{
+				Name: "all-set",
+				CheckRestart: &CheckRestart{
+					Limit:          22,
+					Grace:          helper.TimeToPtr(22 * time.Second),
+					IgnoreWarnings: true,
+				},
+			},
+			{
+				Name: "some-set",
+				CheckRestart: &CheckRestart{
+					Limit: 33,
+					Grace: helper.TimeToPtr(33 * time.Second),
+				},
+			},
+			{
+				Name: "unset",
+			},
+		},
+	}
+
+	service.Canonicalize(task, tg, job)
+	assert.Equal(t, service.Checks[0].CheckRestart.Limit, 22)
+	assert.Equal(t, *service.Checks[0].CheckRestart.Grace, 22*time.Second)
+	assert.True(t, service.Checks[0].CheckRestart.IgnoreWarnings)
+
+	assert.Equal(t, service.Checks[1].CheckRestart.Limit, 33)
+	assert.Equal(t, *service.Checks[1].CheckRestart.Grace, 33*time.Second)
+	assert.True(t, service.Checks[1].CheckRestart.IgnoreWarnings)
+
+	assert.Equal(t, service.Checks[2].CheckRestart.Limit, 11)
+	assert.Equal(t, *service.Checks[2].CheckRestart.Grace, 11*time.Second)
+	assert.True(t, service.Checks[2].CheckRestart.IgnoreWarnings)
+}