Add nodes validation in farmerbot #1135
Conversation
SalmaElsoly
requested review from
muhamadazmy,
mariobassem,
ashraffouda,
rawdaGastan,
Omarabdul3ziz,
AbdelrahmanElawady,
xmonader,
sameh-farouk and
Eslam-Nawara
as code owners
SalmaElsoly
changed the title
farmerbot/parser/parser.go
Outdated
| if _, ok := nodesMap[excludedNode]; !ok { | ||
| return fmt.Errorf("excluded node with id %d doesn't exist in the farm", excludedNode) | ||
| } | ||
| index := slices.Index(input.IncludedNodes, excludedNode) |
farmerbot/cmd/run.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| defer sub.Close() |
There was a problem hiding this comment.
no connection was opened here so no need to close the manager
There was a problem hiding this comment.
Substrate( ) by default returns a connection :
doesn't it make connection to db?
There was a problem hiding this comment.
ahaaa, that's kinda confusing, please create a manager then a connection, or at least rename to subConn, I couldn't see your're using a connection
farmerbot/parser/validator.go
Outdated
| ) | ||
|
|
||
| // ValidateInput validates that included, excluded and priority nodes are in the farm | ||
| func ValidateInput(input *internal.Config, sub internal.Substrate) error { |
There was a problem hiding this comment.
why using the address, why not just passing the value of the config?
There was a problem hiding this comment.
you are right, there is no need for it
farmerbot/cmd/run.go
Outdated
| @@ -41,6 +41,11 @@ var runCmd = &cobra.Command{ | |||
| return err | |||
| } | |||
|
|
|||
| err = parser.ValidateInput(config, network) | |||
There was a problem hiding this comment.
if err := parser.ValidateConfig(config, network); err != nil {
return fmt.Errorf("invalid configuration: %w", err)
}
farmerbot/parser/validator.go
Outdated
| nodesMap[node] = true | ||
| } | ||
| if len(input.IncludedNodes) != 0 { | ||
| //validate included nodes |
There was a problem hiding this comment.
extract the logic into another function and call it here, same for the rest
There was a problem hiding this comment.
what about the testing, split it too?
farmerbot/parser/validator_test.go
Outdated
| ctrl := gomock.NewController(t) | ||
| mockGetNodes := mocks.NewMockSubstrate(ctrl) | ||
| mockGetNodes.EXPECT().GetNodes(config.FarmID).Times(13).Return([]uint32{20, 21, 22, 23, 24, 30, 31, 32, 34, 40, 41}, nil) | ||
| t.Run("test valid include, exclude, priority nodes and never shutdown nodes", func(t *testing.T) { |
There was a problem hiding this comment.
Let's refactor to use e.g Table Driven Tests instead e.g
farmerbot/parser/validator.go
Outdated
| nodesMap[node] = true | ||
| } | ||
| if len(input.IncludedNodes) != 0 { | ||
| if err := validateWithIncludedNodes(input, nodesMap); err != nil { |
There was a problem hiding this comment.
i think what thabet meant was to create a validator function for every set of nodes eg (validateIncludedNodes, validateExcludedNodes,...)
farmerbot/parser/validator.go
Outdated
| return fmt.Errorf("%s node with id %d doesn't exist in the included nodes ", typeOfValidation, node) | ||
| } | ||
| if slices.Contains(excluded, node) { | ||
| return fmt.Errorf("cannot %s and exclude the same node %d", typeOfValidation, node) |
There was a problem hiding this comment.
| return fmt.Errorf("cannot %s and exclude the same node %d", typeOfValidation, node) | |
| return fmt.Errorf("node %d can not be both %s and excluded", node, typeOfValidation) |
farmerbot/parser/validator_test.go
Outdated
| priorityNodes []uint32 | ||
| excludedNodes []uint32 | ||
| neverShutdownNodes []uint32 | ||
| shouldError bool |
farmerbot/parser/validator.go
Outdated
| if slices.Contains(excluded, node) { | ||
| return fmt.Errorf("node %d can not be both %s and excluded", node, typeOfValidation) | ||
| } |
There was a problem hiding this comment.
at this point if the node is in the included nodes, it won't be in the excluded nodes, no?
There was a problem hiding this comment.
yes it will not be in the excluded, this check is added for the case of all nodes included because in that case there is no validation for included nodes so priority and nevershutdown nodes need to be validated against excluded nodes
There was a problem hiding this comment.
then you'd need to remove the excluded nodes from the list of included nodes, check line 41, included nodes should NOT be a map that include excluded nodes
There was a problem hiding this comment.
after that there is no need for this check right?
farmerbot/parser/validator.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func validatePriorityOrNeverShutdown(typeOfValidation string, toBeValidated, excluded []uint32, nodes map[uint32]bool) error { |
There was a problem hiding this comment.
rename nodes to includedNodes
farmerbot/parser/validator.go
Outdated
| if len(input.IncludedNodes) != 0 { | ||
| if err := validateIncludedNodes(input.IncludedNodes, input.ExcludedNodes, nodesMap); err != nil { | ||
| return err | ||
| } | ||
| for _, includedNode := range input.IncludedNodes { | ||
| includedNodes[includedNode] = true | ||
| } | ||
| } else { | ||
| for key, value := range nodesMap { | ||
| if slices.Contains(input.ExcludedNodes, key) { | ||
| continue | ||
| } | ||
| includedNodes[key] = value | ||
| } | ||
| } |
There was a problem hiding this comment.
| if len(input.IncludedNodes) != 0 { | |
| if err := validateIncludedNodes(input.IncludedNodes, input.ExcludedNodes, nodesMap); err != nil { | |
| return err | |
| } | |
| for _, includedNode := range input.IncludedNodes { | |
| includedNodes[includedNode] = true | |
| } | |
| } else { | |
| for key, value := range nodesMap { | |
| if slices.Contains(input.ExcludedNodes, key) { | |
| continue | |
| } | |
| includedNodes[key] = value | |
| } | |
| } | |
| for _, includedNode := range input.IncludedNodes { | |
| includedNodes[includedNode] = true | |
| } | |
| if len(icludedNodes == 0){ | |
| for key, value := range nodesMap { | |
| if !slices.Contains(input.ExcludedNodes, key) { | |
| includedNodes[key] = value | |
| } | |
| } | |
| } | |
| if err := validateIncludedNodes(input.IncludedNodes, input.ExcludedNodes, nodesMap); err != nil { | |
| return err | |
| } | |
farmerbot/parser/validator.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("couldn't retrieve node for %d : %v", input.FarmID, err) | ||
| } | ||
| nodesMap := make(map[uint32]bool) |
Description
Describe the changes introduced by this PR and what does it affect
Changes
List of changes this PR includes
ensuring that each of them is included in the farm, no overlapping between included and excluded nodes
Related Issues
List of related issues
Checklist