Cleanup of rancher server command#349
Merged
enrichman merged 10 commits intorancher:v2.9from Feb 13, 2024
Merged
Conversation
JonCrowther
reviewed
Feb 9, 2024
Contributor
JonCrowther
left a comment
There was a problem hiding this comment.
Overall looks very good, just a couple questions for my understanding.
JonCrowther
approved these changes
Feb 12, 2024
Contributor
JonCrowther
left a comment
There was a problem hiding this comment.
This LGTM. Thank you for adding the changes I suggested!
crobby
approved these changes
Feb 13, 2024
Contributor
crobby
left a comment
There was a problem hiding this comment.
lgtm, happy to see some tests in here!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes rancher/rancher#44374
This PR contains the refactor of the #347.
The panics fix where added with #348
Description
Fixing the panics I took the opportunity to cleanup a bit the code (only the
servercommand, to avoid a huge and "critical" refactor). The server command is pretty safe to play with.Also some deprecated deps were updated (
terminalandioutil).1) Common validation
The server commands are expecting a valid server configuration:
This common validation was changed to a
cli.BeforeFuncand added in theBeforeof theservercommands.2) Testing
To make test possible I moved the business logic to their dedicated func, dropping (when possible) the dependency from the
cli.Context.I.e.: the
ServerCurrentnow accept anio.Writerand a*config.Config. Or theServerSwitchnow needs only the*config.Configand theserverName. This made test possible (and tests were added).3) Centralized Config
The
*config.Configis needed by every commands, and loaded every time in the same (?) way. This can be done only once at the very start of the root command, in the BeforeFunc. Then the Config can be just passed as a dependency to the commands.Be aware that to make this work a pointer has to be passed downward, and update the underlying struct in the Before. This is because in Go arguments are passe by value, and the Commands are built before the actual Run (obviously). So the pointer will be always the same, but it will point to the just loaded conf.
With this approach we can drop all the
loadConfigin every command:4) Simplifications
Some minor cleanup, like the
getServers(cfg *config.Config) []*serverDatafunc.Instead of getting the ordered keys, and then loop through them we can simply use the already existing
serverDatastruct.This will return an ordered slice of
serverDatathat will be used by the server prompt selection, and also by theServerLs, that was using an unordered set of servers (now it's consistent).