Skip to content

Commit

Permalink
update from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@nvanthao
nvanthao committed Oct 21, 2024
1 parent 7683c8d commit 72d375c
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 40 deletions.
18 changes: 9 additions & 9 deletions pkg/analyze/collected_contents.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,26 @@ import (
"github.com/replicatedhq/troubleshoot/pkg/constants"
)

type CollectedContent struct {
type collectedContent struct {
NodeName string
Data CollectorData
Data collectorData
}

type CollectorData interface{}
type collectorData interface{}

type NodeNames struct {
type nodeNames struct {
Nodes []string `json:"nodes"`
}

func retrieveCollectedContents(
getCollectedFileContents func(string) ([]byte, error),
localPath string, remoteNodeBaseDir string, remoteFileName string,
) ([]CollectedContent, error) {
var collectedContents []CollectedContent
) ([]collectedContent, error) {
var collectedContents []collectedContent

// Try to retrieve local data first
if contents, err := getCollectedFileContents(localPath); err == nil {
collectedContents = append(collectedContents, CollectedContent{NodeName: "", Data: contents})
collectedContents = append(collectedContents, collectedContent{NodeName: "", Data: contents})
// Return immediately if local content is available
return collectedContents, nil
}
Expand All @@ -38,7 +38,7 @@ func retrieveCollectedContents(
return nil, errors.Wrap(err, "failed to get node list")
}

var nodeNames NodeNames
var nodeNames nodeNames
if err := json.Unmarshal(nodeListContents, &nodeNames); err != nil {
return nil, errors.Wrap(err, "failed to unmarshal node names")
}
Expand All @@ -51,7 +51,7 @@ func retrieveCollectedContents(
return nil, errors.Wrapf(err, "failed to retrieve content for node %s", node)
}

collectedContents = append(collectedContents, CollectedContent{NodeName: node, Data: nodeContents})
collectedContents = append(collectedContents, collectedContent{NodeName: node, Data: nodeContents})
}

return collectedContents, nil
Expand Down
10 changes: 5 additions & 5 deletions pkg/analyze/collected_contents_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func TestRetrieveCollectedContents(t *testing.T) {
localPath string
remoteNodeBaseDir string
remoteFileName string
expectedResult []CollectedContent
expectedResult []collectedContent
expectedError string
}{
{
Expand All @@ -31,7 +31,7 @@ func TestRetrieveCollectedContents(t *testing.T) {
localPath: "localPath",
remoteNodeBaseDir: "remoteBaseDir",
remoteFileName: "remoteFileName",
expectedResult: []CollectedContent{
expectedResult: []collectedContent{
{
NodeName: "",
Data: []byte("localContent"),
Expand All @@ -43,7 +43,7 @@ func TestRetrieveCollectedContents(t *testing.T) {
name: "local content not found, retrieve remote node content successfully",
getCollectedFileContents: func(path string) ([]byte, error) {
if path == constants.NODE_LIST_FILE {
nodeNames := NodeNames{Nodes: []string{"node1", "node2"}}
nodeNames := nodeNames{Nodes: []string{"node1", "node2"}}
return json.Marshal(nodeNames)
}
if path == "remoteBaseDir/node1/remoteFileName" {
Expand All @@ -57,7 +57,7 @@ func TestRetrieveCollectedContents(t *testing.T) {
localPath: "localPath",
remoteNodeBaseDir: "remoteBaseDir",
remoteFileName: "remoteFileName",
expectedResult: []CollectedContent{
expectedResult: []collectedContent{
{
NodeName: "node1",
Data: []byte("remoteContent1"),
Expand All @@ -84,7 +84,7 @@ func TestRetrieveCollectedContents(t *testing.T) {
name: "fail to retrieve content for one of the nodes",
getCollectedFileContents: func(path string) ([]byte, error) {
if path == constants.NODE_LIST_FILE {
nodeNames := NodeNames{Nodes: []string{"node1", "node2"}}
nodeNames := nodeNames{Nodes: []string{"node1", "node2"}}
return json.Marshal(nodeNames)
}
if path == "remoteBaseDir/node1/remoteFileName" {
Expand Down
8 changes: 5 additions & 3 deletions pkg/analyze/host_analyzer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (c *resultCollector) get(title string) []*AnalyzeResult {
return []*AnalyzeResult{{Title: title, IsWarn: true, Message: "no results"}}
}

func analyzeHostCollectorResults(collectedContent []CollectedContent, outcomes []*troubleshootv1beta2.Outcome, checkCondition func(string, CollectorData) (bool, error), title string) ([]*AnalyzeResult, error) {
func analyzeHostCollectorResults(collectedContent []collectedContent, outcomes []*troubleshootv1beta2.Outcome, checkCondition func(string, collectorData) (bool, error), title string) ([]*AnalyzeResult, error) {
var results []*AnalyzeResult
for _, content := range collectedContent {
currentTitle := title
Expand All @@ -101,12 +101,14 @@ func analyzeHostCollectorResults(collectedContent []CollectedContent, outcomes [
if err != nil {
return nil, errors.Wrap(err, "failed to evaluate outcomes")
}
results = append(results, analyzeResult...)
if analyzeResult != nil {
results = append(results, analyzeResult...)
}
}
return results, nil
}

func evaluateOutcomes(outcomes []*troubleshootv1beta2.Outcome, checkCondition func(string, CollectorData) (bool, error), data CollectorData, title string) ([]*AnalyzeResult, error) {
func evaluateOutcomes(outcomes []*troubleshootv1beta2.Outcome, checkCondition func(string, collectorData) (bool, error), data collectorData, title string) ([]*AnalyzeResult, error) {
var results []*AnalyzeResult

for _, outcome := range outcomes {
Expand Down
30 changes: 15 additions & 15 deletions pkg/analyze/host_analyzer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ func TestAnalyzeHostCollectorResults(t *testing.T) {
tests := []struct {
name string
outcomes []*troubleshootv1beta2.Outcome
collectedContent []CollectedContent
checkCondition func(string, CollectorData) (bool, error)
collectedContent []collectedContent
checkCondition func(string, collectorData) (bool, error)
expectResult []*AnalyzeResult
}{
{
name: "pass if ubuntu >= 00.1.2",
collectedContent: []CollectedContent{
collectedContent: []collectedContent{
{
NodeName: "node1",
Data: collect.HostOSInfo{
Expand All @@ -44,7 +44,7 @@ func TestAnalyzeHostCollectorResults(t *testing.T) {
},
},
},
checkCondition: func(when string, data CollectorData) (bool, error) {
checkCondition: func(when string, data collectorData) (bool, error) {
osInfo := data.(collect.HostOSInfo)
return osInfo.Platform == "ubuntu" && osInfo.PlatformVersion >= "00.1.2", nil
},
Expand All @@ -58,7 +58,7 @@ func TestAnalyzeHostCollectorResults(t *testing.T) {
},
{
name: "fail if ubuntu <= 11.04",
collectedContent: []CollectedContent{
collectedContent: []collectedContent{
{
NodeName: "node1",
Data: collect.HostOSInfo{
Expand Down Expand Up @@ -91,7 +91,7 @@ func TestAnalyzeHostCollectorResults(t *testing.T) {
},
},
},
checkCondition: func(when string, data CollectorData) (bool, error) {
checkCondition: func(when string, data collectorData) (bool, error) {
osInfo := data.(collect.HostOSInfo)
return osInfo.Platform == "ubuntu" && osInfo.PlatformVersion <= "11.04", nil
},
Expand All @@ -110,7 +110,7 @@ func TestAnalyzeHostCollectorResults(t *testing.T) {
},
{
name: "title does not include node name if empty",
collectedContent: []CollectedContent{
collectedContent: []collectedContent{
{
NodeName: "",
Data: collect.HostOSInfo{
Expand All @@ -134,7 +134,7 @@ func TestAnalyzeHostCollectorResults(t *testing.T) {
},
},
},
checkCondition: func(when string, data CollectorData) (bool, error) {
checkCondition: func(when string, data collectorData) (bool, error) {
osInfo := data.(collect.HostOSInfo)
return osInfo.Platform == "ubuntu" && osInfo.PlatformVersion >= "20.04", nil
},
Expand Down Expand Up @@ -162,8 +162,8 @@ func TestEvaluateOutcomes(t *testing.T) {
tests := []struct {
name string
outcomes []*troubleshootv1beta2.Outcome
checkCondition func(string, CollectorData) (bool, error)
data CollectorData
checkCondition func(string, collectorData) (bool, error)
data collectorData
expectedResult []*AnalyzeResult
}{
{
Expand All @@ -176,7 +176,7 @@ func TestEvaluateOutcomes(t *testing.T) {
},
},
},
checkCondition: func(when string, data CollectorData) (bool, error) {
checkCondition: func(when string, data collectorData) (bool, error) {
// Return true if the condition being checked matches "failCondition"
return when == "failCondition", nil
},
Expand All @@ -199,7 +199,7 @@ func TestEvaluateOutcomes(t *testing.T) {
},
},
},
checkCondition: func(when string, data CollectorData) (bool, error) {
checkCondition: func(when string, data collectorData) (bool, error) {
// Return true if the condition being checked matches "warnCondition"
return when == "warnCondition", nil
},
Expand All @@ -222,7 +222,7 @@ func TestEvaluateOutcomes(t *testing.T) {
},
},
},
checkCondition: func(when string, data CollectorData) (bool, error) {
checkCondition: func(when string, data collectorData) (bool, error) {
// Return true if the condition being checked matches "passCondition"
return when == "passCondition", nil
},
Expand Down Expand Up @@ -253,7 +253,7 @@ func TestEvaluateOutcomes(t *testing.T) {
},
},
},
checkCondition: func(when string, data CollectorData) (bool, error) {
checkCondition: func(when string, data collectorData) (bool, error) {
// Always return false to simulate no condition matching
return false, nil
},
Expand All @@ -270,7 +270,7 @@ func TestEvaluateOutcomes(t *testing.T) {
},
},
},
checkCondition: func(when string, data CollectorData) (bool, error) {
checkCondition: func(when string, data collectorData) (bool, error) {
// Simulate an error occurring during condition evaluation
return false, errors.New("mock error")
},
Expand Down
2 changes: 1 addition & 1 deletion pkg/analyze/host_memory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (a *AnalyzeHostMemory) Analyze(
}

// checkCondition checks the condition of the when clause
func (a *AnalyzeHostMemory) CheckCondition(when string, data CollectorData) (bool, error) {
func (a *AnalyzeHostMemory) CheckCondition(when string, data collectorData) (bool, error) {
rawData, ok := data.([]byte)
if !ok {
return false, fmt.Errorf("expected data to be []uint8 (raw bytes), got: %v", reflect.TypeOf(data))
Expand Down
4 changes: 2 additions & 2 deletions pkg/analyze/host_memory_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func TestAnalyzeHostMemory(t *testing.T) {
getCollectedFileContents: func(path string) ([]byte, error) {
// Simulate remote node list and memory content retrieval
if path == constants.NODE_LIST_FILE {
nodeNames := NodeNames{Nodes: []string{"node1"}}
nodeNames := nodeNames{Nodes: []string{"node1"}}
return json.Marshal(nodeNames)
}
if path == fmt.Sprintf("%s/node1/%s", collect.NodeInfoBaseDir, collect.HostMemoryFileName) {
Expand Down Expand Up @@ -185,7 +185,7 @@ func TestAnalyzeHostMemory(t *testing.T) {
getCollectedFileContents: func(path string) ([]byte, error) {
// Simulate remote node list and memory content retrieval
if path == constants.NODE_LIST_FILE {
nodeNames := NodeNames{Nodes: []string{"node1"}}
nodeNames := nodeNames{Nodes: []string{"node1"}}
return json.Marshal(nodeNames)
}
if path == fmt.Sprintf("%s/node1/%s", collect.NodeInfoBaseDir, collect.HostMemoryFileName) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/analyze/host_os_info.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (a *AnalyzeHostOS) Analyze(
}

// checkCondition checks the condition of the when clause
func (a *AnalyzeHostOS) CheckCondition(when string, data CollectorData) (bool, error) {
func (a *AnalyzeHostOS) CheckCondition(when string, data collectorData) (bool, error) {
rawData, ok := data.([]byte)
if !ok {
return false, fmt.Errorf("expected data to be []uint8 (raw bytes), got: %v", reflect.TypeOf(data))
Expand Down
8 changes: 4 additions & 4 deletions pkg/analyze/host_os_info_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func TestAnalyzeHostOS(t *testing.T) {
},
getCollectedFileContents: func(path string) ([]byte, error) {
if path == constants.NODE_LIST_FILE {
return json.Marshal(NodeNames{Nodes: []string{"node1"}})
return json.Marshal(nodeNames{Nodes: []string{"node1"}})
}
if path == fmt.Sprintf("%s/node1/%s", collect.NodeInfoBaseDir, collect.HostInfoFileName) {
return json.Marshal(collect.HostOSInfo{
Expand Down Expand Up @@ -211,7 +211,7 @@ func TestAnalyzeHostOS(t *testing.T) {
},
getCollectedFileContents: func(path string) ([]byte, error) {
if path == constants.NODE_LIST_FILE {
return json.Marshal(NodeNames{Nodes: []string{"node1"}})
return json.Marshal(nodeNames{Nodes: []string{"node1"}})
}
if path == fmt.Sprintf("%s/node1/%s", collect.NodeInfoBaseDir, collect.HostInfoFileName) {
return nil, errors.New("file not found")
Expand Down Expand Up @@ -240,7 +240,7 @@ func TestAnalyzeHostOS(t *testing.T) {
},
getCollectedFileContents: func(path string) ([]byte, error) {
if path == constants.NODE_LIST_FILE {
return json.Marshal(NodeNames{Nodes: []string{"node1"}})
return json.Marshal(nodeNames{Nodes: []string{"node1"}})
}
if path == fmt.Sprintf("%s/node1/%s", collect.NodeInfoBaseDir, collect.HostInfoFileName) {
return json.Marshal(collect.HostOSInfo{
Expand Down Expand Up @@ -318,7 +318,7 @@ func TestAnalyzeHostOS(t *testing.T) {
},
getCollectedFileContents: func(path string) ([]byte, error) {
if path == constants.NODE_LIST_FILE {
return json.Marshal(NodeNames{Nodes: []string{"node1", "node2"}})
return json.Marshal(nodeNames{Nodes: []string{"node1", "node2"}})
}
if path == fmt.Sprintf("%s/node1/%s", collect.NodeInfoBaseDir, collect.HostInfoFileName) {
return json.Marshal(collect.HostOSInfo{
Expand Down

0 comments on commit 72d375c

Please sign in to comment.