Refactor integration commands into package#21
Conversation
1030de7 to
c7d356b
Compare
ChezCrawford
left a comment
There was a problem hiding this comment.
In general this looks great. The tests are super nice. Left a few comments to have a look at...
| return nil | ||
| } | ||
|
|
||
| func RunRetryCommand(config *Config, routingKey string) error { |
There was a problem hiding this comment.
I'm not sure we need to share most of these...
There was a problem hiding this comment.
Good point, since this is basically all the same feedback I'll resolve the rest of these and put the commit here: 25853c8
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| viper.SetDefault("address", cmdutil.GetDefaults().Address) |
There was a problem hiding this comment.
I'm interested in why this is required here. (I see that if we remove it, the tests don't work but i'd like to understand why.)
As I am looking at this, i'm wondering if we also might want to spend a little time refactoring cmdutil.Defaults and cmdutil.Config
There was a problem hiding this comment.
Yeah for sure I'm also confused why this is necessary here, and not in the cmd tests. I did spend an hour or so trying to figure this out, but I'll put some more time into this today to see if I can get a definitive answer because this obviously isn't ideal.
To comment on the second part, I'm not really sure what you mean about refactoring cmdutil.Defaults and cmdutil.Config, could you elaborate a bit on what you'd want the refactor to look like?
There was a problem hiding this comment.
Good news! I figured why address was not getting set in the nagios package tests. The cmd package has an init function that gets run whenever the package is used, so in cmd tests the viper stuff gets initialized but this won't happen in the tests for the integrations because they'll be in their own packages. I pushed a few commits trying some things but this is the final solution I ended on. Lmk what you think: 61699b2
There was a problem hiding this comment.
The solution you've put in place here is better.
As we discussed, we may want to do some further refactoring in the future to pull viper and its side-effects further to the edges of the application and have the inner layers access config through a config object.
This refactor should make adding new tests easier and more closely follows established Go testing patterns.
We now store the command's inputs in a new type, and do a direct mapping from the new type to a V2 event type. This should make the code more readable.
82282cd to
2fbf2af
Compare
2fbf2af to
61699b2
Compare
No description provided.