fixed issue where warnings are disseminated along with passes (#390)

murphybytes · web-flow · commit 6007f152535f · 2021-07-22T08:27:39.000-05:00
diff --git a/pkg/analyze/host_filesystem_performance.go b/pkg/analyze/host_filesystem_performance.go
@@ -45,19 +45,19 @@ func (a *AnalyzeHostFilesystemPerformance) Analyze(getCollectedFileContents func
 		return nil, errors.Wrapf(err, "failed to unmarshal filesystem performance results from %s", name)
 	}
 
-	var coll resultCollector
+	result := &AnalyzeResult{
+		Title: a.Title(),
+	}
 
 	for _, outcome := range hostAnalyzer.Outcomes {
-		result := &AnalyzeResult{Title: a.Title()}
 
 		if outcome.Fail != nil {
 			if outcome.Fail.When == "" {
 				result.IsFail = true
 				result.Message = renderFSPerfOutcome(outcome.Fail.Message, fsPerf)
 				result.URI = outcome.Fail.URI
 
-				coll.push(result)
-				continue
+				return []*AnalyzeResult{result}, nil
 			}
 
 			isMatch, err := compareHostFilesystemPerformanceConditionalToActual(outcome.Fail.When, fsPerf)
@@ -70,16 +70,15 @@ func (a *AnalyzeHostFilesystemPerformance) Analyze(getCollectedFileContents func
 				result.Message = renderFSPerfOutcome(outcome.Fail.Message, fsPerf)
 				result.URI = outcome.Fail.URI
 
-				coll.push(result)
+				return []*AnalyzeResult{result}, nil
 			}
 		} else if outcome.Warn != nil {
 			if outcome.Warn.When == "" {
 				result.IsWarn = true
 				result.Message = renderFSPerfOutcome(outcome.Warn.Message, fsPerf)
 				result.URI = outcome.Warn.URI
 
-				coll.push(result)
-				continue
+				return []*AnalyzeResult{result}, nil
 			}
 
 			isMatch, err := compareHostFilesystemPerformanceConditionalToActual(outcome.Warn.When, fsPerf)
@@ -92,16 +91,15 @@ func (a *AnalyzeHostFilesystemPerformance) Analyze(getCollectedFileContents func
 				result.Message = renderFSPerfOutcome(outcome.Warn.Message, fsPerf)
 				result.URI = outcome.Warn.URI
 
-				coll.push(result)
+				return []*AnalyzeResult{result}, nil
 			}
 		} else if outcome.Pass != nil {
 			if outcome.Pass.When == "" {
 				result.IsPass = true
 				result.Message = renderFSPerfOutcome(outcome.Pass.Message, fsPerf)
 				result.URI = outcome.Pass.URI
 
-				coll.push(result)
-				continue
+				return []*AnalyzeResult{result}, nil
 			}
 
 			isMatch, err := compareHostFilesystemPerformanceConditionalToActual(outcome.Pass.When, fsPerf)
@@ -114,13 +112,13 @@ func (a *AnalyzeHostFilesystemPerformance) Analyze(getCollectedFileContents func
 				result.Message = renderFSPerfOutcome(outcome.Pass.Message, fsPerf)
 				result.URI = outcome.Pass.URI
 
-				coll.push(result)
+				return []*AnalyzeResult{result}, nil
 			}
 
 		}
 	}
 
-	return coll.get(a.Title()), nil
+	return []*AnalyzeResult{result}, nil
 }
 
 func compareHostFilesystemPerformanceConditionalToActual(conditional string, fsPerf collect.FSPerfResults) (res bool, err error) {
diff --git a/pkg/analyze/host_filesystem_performance_test.go b/pkg/analyze/host_filesystem_performance_test.go
@@ -296,6 +296,42 @@ func TestAnalyzeHostFilesystemPerformance(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "skip warn if pass first",
+			fsPerf: &collect.FSPerfResults{
+				P99: 9 * time.Millisecond,
+			},
+			hostAnalyzer: &troubleshootv1beta2.FilesystemPerformanceAnalyze{
+				CollectorName: "file system performance",
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Pass: &troubleshootv1beta2.SingleOutcome{
+							When:    "p99 < 10ms",
+							Message: "Acceptable write latency",
+						},
+					},
+					{
+						Warn: &troubleshootv1beta2.SingleOutcome{
+							When:    "p99 < 20ms",
+							Message: "Warn write latency",
+						},
+					},
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							When:    "p99 >= 20ms",
+							Message: "fail",
+						},
+					},
+				},
+			},
+			result: []*AnalyzeResult{
+				{
+					Title:   "Filesystem Performance",
+					IsPass:  true,
+					Message: "Acceptable write latency",
+				},
+			},
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
diff --git a/pkg/analyze/host_tcpportstatus.go b/pkg/analyze/host_tcpportstatus.go
@@ -10,6 +10,8 @@ import (
 	"github.com/replicatedhq/troubleshoot/pkg/collect"
 )
 
+// AnalyzeHostTCPPortStatus is an analyzer that will return only one matching result, or a warning if nothing matches. The first
+// match that is encountered is the one that is returned.
 type AnalyzeHostTCPPortStatus struct {
 	hostAnalyzer *troubleshootv1beta2.TCPPortStatusAnalyze
 }
@@ -39,61 +41,60 @@ func (a *AnalyzeHostTCPPortStatus) Analyze(getCollectedFileContents func(string)
 		return nil, errors.Wrap(err, "failed to unmarshal collected")
 	}
 
-	var coll resultCollector
+	result := &AnalyzeResult{Title: a.Title()}
 
 	for _, outcome := range hostAnalyzer.Outcomes {
-		result := &AnalyzeResult{Title: a.Title()}
 
 		if outcome.Fail != nil {
 			if outcome.Fail.When == "" {
 				result.IsFail = true
 				result.Message = outcome.Fail.Message
 				result.URI = outcome.Fail.URI
 
-				coll.push(result)
+				return []*AnalyzeResult{result}, nil
 			}
 
 			if string(actual.Status) == outcome.Fail.When {
 				result.IsFail = true
 				result.Message = outcome.Fail.Message
 				result.URI = outcome.Fail.URI
 
-				coll.push(result)
+				return []*AnalyzeResult{result}, nil
 			}
 		} else if outcome.Warn != nil {
 			if outcome.Warn.When == "" {
 				result.IsWarn = true
 				result.Message = outcome.Warn.Message
 				result.URI = outcome.Warn.URI
 
-				coll.push(result)
+				return []*AnalyzeResult{result}, nil
 			}
 
 			if string(actual.Status) == outcome.Warn.When {
 				result.IsWarn = true
 				result.Message = outcome.Warn.Message
 				result.URI = outcome.Warn.URI
 
-				coll.push(result)
+				return []*AnalyzeResult{result}, nil
 			}
 		} else if outcome.Pass != nil {
 			if outcome.Pass.When == "" {
 				result.IsPass = true
 				result.Message = outcome.Pass.Message
 				result.URI = outcome.Pass.URI
 
-				coll.push(result)
+				return []*AnalyzeResult{result}, nil
 			}
 
 			if string(actual.Status) == outcome.Pass.When {
 				result.IsPass = true
 				result.Message = outcome.Pass.Message
 				result.URI = outcome.Pass.URI
 
-				coll.push(result)
+				return []*AnalyzeResult{result}, nil
 			}
 		}
 	}
 
-	return coll.get(a.Title()), nil
+	return []*AnalyzeResult{result}, nil
 }
diff --git a/pkg/analyze/host_tcpportstatus_test.go b/pkg/analyze/host_tcpportstatus_test.go
@@ -0,0 +1,104 @@
+package analyzer
+
+import (
+	"encoding/json"
+	"testing"
+
+	troubleshootv1beta2 "github.com/replicatedhq/troubleshoot/pkg/apis/troubleshoot/v1beta2"
+	"github.com/replicatedhq/troubleshoot/pkg/collect"
+	"github.com/stretchr/testify/require"
+)
+
+func TestAnalyzeTCPPortStatus(t *testing.T) {
+	tt := []struct {
+		name      string
+		collected collect.NetworkStatusResult
+		analyzer  troubleshootv1beta2.TCPPortStatusAnalyze
+		results   []*AnalyzeResult
+		wantErr   bool
+	}{
+		{
+			name: "pass expect single result",
+			collected: collect.NetworkStatusResult{
+				Status: "connected",
+			},
+			analyzer: troubleshootv1beta2.TCPPortStatusAnalyze{
+				AnalyzeMeta: troubleshootv1beta2.AnalyzeMeta{
+					CheckName: "Kubernetes API TCP Port Status",
+				},
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Pass: &troubleshootv1beta2.SingleOutcome{
+							When:    "connected",
+							Message: "Port 6443 is open",
+						},
+					},
+					{
+						Warn: &troubleshootv1beta2.SingleOutcome{
+							Message: "Unexpected port status",
+						},
+					},
+				},
+			},
+			results: []*AnalyzeResult{
+				{
+					Title:   "Kubernetes API TCP Port Status",
+					IsPass:  true,
+					Message: "Port 6443 is open",
+				},
+			},
+		},
+
+		{
+			name: "get warn if no match",
+			collected: collect.NetworkStatusResult{
+				Status: "connected",
+			},
+			analyzer: troubleshootv1beta2.TCPPortStatusAnalyze{
+				AnalyzeMeta: troubleshootv1beta2.AnalyzeMeta{
+					CheckName: "Kubernetes API TCP Port Status",
+				},
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							When:    "foo",
+							Message: "Port 6443 is open",
+						},
+					},
+					{
+						Warn: &troubleshootv1beta2.SingleOutcome{
+							Message: "Unexpected port status",
+						},
+					},
+				},
+			},
+			results: []*AnalyzeResult{
+				{
+					Title:   "Kubernetes API TCP Port Status",
+					IsWarn:  true,
+					Message: "Unexpected port status",
+				},
+			},
+		},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			fn := func(_ string) ([]byte, error) {
+				return json.Marshal(&tc.collected)
+			}
+			analyzer := AnalyzeHostTCPPortStatus{
+				hostAnalyzer: &tc.analyzer,
+			}
+			results, err := analyzer.Analyze(fn)
+			if tc.wantErr {
+				require.NotNil(t, err)
+				return
+			}
+
+			require.Nil(t, err)
+			require.Equal(t, tc.results, results)
+		})
+	}
+
+}

Original file line number	Diff line number	Diff line change
`@@ -10,6 +10,8 @@ import (`
`10`	`10`	`"github.com/replicatedhq/troubleshoot/pkg/collect"`
`11`	`11`	`)`
`12`	`12`
	`13`	`+// AnalyzeHostTCPPortStatus is an analyzer that will return only one matching result, or a warning if nothing matches. The first`
	`14`	`+// match that is encountered is the one that is returned.`
`13`	`15`	`type AnalyzeHostTCPPortStatus struct {`
`14`	`16`	`hostAnalyzer *troubleshootv1beta2.TCPPortStatusAnalyze`
`15`	`17`	`}`
`@@ -39,61 +41,60 @@ func (a *AnalyzeHostTCPPortStatus) Analyze(getCollectedFileContents func(string)`
`39`	`41`	`return nil, errors.Wrap(err, "failed to unmarshal collected")`
`40`	`42`	`}`
`41`	`43`
`42`		`- var coll resultCollector`
	`44`	`+ result := &AnalyzeResult{Title: a.Title()}`
`43`	`45`
`44`	`46`	`for _, outcome := range hostAnalyzer.Outcomes {`
`45`		`- result := &AnalyzeResult{Title: a.Title()}`
`46`	`47`
`47`	`48`	`if outcome.Fail != nil {`
`48`	`49`	`if outcome.Fail.When == "" {`
`49`	`50`	`result.IsFail = true`
`50`	`51`	`result.Message = outcome.Fail.Message`
`51`	`52`	`result.URI = outcome.Fail.URI`
`52`	`53`
`53`		`- coll.push(result)`
	`54`	`+ return []*AnalyzeResult{result}, nil`
`54`	`55`	`}`
`55`	`56`
`56`	`57`	`if string(actual.Status) == outcome.Fail.When {`
`57`	`58`	`result.IsFail = true`
`58`	`59`	`result.Message = outcome.Fail.Message`
`59`	`60`	`result.URI = outcome.Fail.URI`
`60`	`61`
`61`		`- coll.push(result)`
	`62`	`+ return []*AnalyzeResult{result}, nil`
`62`	`63`	`}`
`63`	`64`	`} else if outcome.Warn != nil {`
`64`	`65`	`if outcome.Warn.When == "" {`
`65`	`66`	`result.IsWarn = true`
`66`	`67`	`result.Message = outcome.Warn.Message`
`67`	`68`	`result.URI = outcome.Warn.URI`
`68`	`69`
`69`		`- coll.push(result)`
	`70`	`+ return []*AnalyzeResult{result}, nil`
`70`	`71`	`}`
`71`	`72`
`72`	`73`	`if string(actual.Status) == outcome.Warn.When {`
`73`	`74`	`result.IsWarn = true`
`74`	`75`	`result.Message = outcome.Warn.Message`
`75`	`76`	`result.URI = outcome.Warn.URI`
`76`	`77`
`77`		`- coll.push(result)`
	`78`	`+ return []*AnalyzeResult{result}, nil`
`78`	`79`	`}`
`79`	`80`	`} else if outcome.Pass != nil {`
`80`	`81`	`if outcome.Pass.When == "" {`
`81`	`82`	`result.IsPass = true`
`82`	`83`	`result.Message = outcome.Pass.Message`
`83`	`84`	`result.URI = outcome.Pass.URI`
`84`	`85`
`85`		`- coll.push(result)`
	`86`	`+ return []*AnalyzeResult{result}, nil`
`86`	`87`	`}`
`87`	`88`
`88`	`89`	`if string(actual.Status) == outcome.Pass.When {`
`89`	`90`	`result.IsPass = true`
`90`	`91`	`result.Message = outcome.Pass.Message`
`91`	`92`	`result.URI = outcome.Pass.URI`
`92`	`93`
`93`		`- coll.push(result)`
	`94`	`+ return []*AnalyzeResult{result}, nil`
`94`	`95`	`}`
`95`	`96`	`}`
`96`	`97`	`}`
`97`	`98`
`98`		`- return coll.get(a.Title()), nil`
	`99`	`+ return []*AnalyzeResult{result}, nil`
`99`	`100`	`}`