-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add helper functions for e2e tests #198
Conversation
8eb2046
to
7758b80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, although mostly naming nits, the refactors/new function look good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments above.
7758b80
to
6e25a5f
Compare
return | ||
} | ||
|
||
func (f *Framework) SetGatewayLabelOnNode(cluster ClusterIndex, nodeName string, isGateway bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a "UnsetGatewayLabelOnNode" for remove cluster test use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have 2 separate functions, one to set to true, other set to false instead of 1. 2 ways of doing it - matter of opinion I guess. I think the single function will suffice - we'd still need it anyway for reuse with the set-to-true and set-to-false functions as syntactic sugar on top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the same function with isGateway "true" or "false" is enough. Renaming the function to "ToggleGatewayLabelOnNode" would be more appropriate.
This is not something that needs urgent change.
Rest of the patch looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set is better here, toggle implies that you don’t pass true/false and It will switch the existing state.
Added some helper functions for upcoming e2e enhancements. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
6e25a5f
to
73861c8
Compare
- Renamed FindNodes to FindNodesByGatewayLabel and changed to return an array of Nodes - Added function doc coomments Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Pushed 1d41096 to change |
return | ||
} | ||
|
||
func (f *Framework) SetGatewayLabelOnNode(cluster ClusterIndex, nodeName string, isGateway bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the same function with isGateway "true" or "false" is enough. Renaming the function to "ToggleGatewayLabelOnNode" would be more appropriate.
This is not something that needs urgent change.
Rest of the patch looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot Thomas!
Added some helper functions for upcoming e2e enhancements.
Signed-off-by: Tom Pantelis tompantelis@gmail.com