feat(newrelic): Display deploy information from multiple apps #472
Conversation
There was a problem hiding this comment.
Good first pass! Reviewing the code, I think there are some performance issues that arise from migrating from the widget being designed with one in mind to supporting multiple instances. Minimally, I think the Refresh one needs addressing, and we can refactor some of the other parts iteratively.
| import "github.com/gdamore/tcell" | ||
|
|
||
| func (widget *Widget) initializeKeyboardControls() { | ||
| widget.SetKeyboardChar("/", widget.ShowHelp, "Show/hide this help window") |
There was a problem hiding this comment.
Can you take a look at the JIRA module and add appropriate keyboard commands? r/j/k would be appreciated for consistency.
| deployCount: ymlConfig.UInt("deployCount", 5), | ||
| apiKey: ymlConfig.UString("apiKey", os.Getenv("WTF_NEW_RELIC_API_KEY")), | ||
| deployCount: ymlConfig.UInt("deployCount", 5), | ||
| applicationIDs: ymlConfig.UList("applicationIDs"), |
There was a problem hiding this comment.
You could keep applicationID for a non-breaking change. Either by appending it to applicationIDs, or doing something like the arrayifyProjects in the jira settings file
| if appErr == nil { | ||
| appName = app.Name | ||
| for _, id := range wtf.ToInts(widget.settings.applicationIDs) { | ||
| widget.Clients = append(widget.Clients, NewClient(widget.settings.apiKey, id)) |
There was a problem hiding this comment.
This will create a new client that is calling the service each time, and this is probably not quite what you want. Say you had 3 applications, on the first refresh there would be 3, second would be 6, third would be 9..... eventually this will eat up a lot of resources.
It might make more sense to have the client accept applicationId as a parameter, and use that to iterate through requests and store the results in some struct, effectively caching the results. Otherwise, every time you change a page you will ALSO be making a web request to the newrelic API to get data, resulting in slower transitions. Minimally, I would create one set of clients up front, and Refresh will just call the display method
|
@ChrisDBrown If there's no feedback from you by Aug 4 on the comments in this PR I'll assume it's unmaintained and will close it out as such. |
|
I'm happy to refactor some of this to address my comments, either as part of this PR, or opening a new one. This IS most of the way done it seems. Though I don't have a new relic account to test |
|
Added via #607 |
Closes #471
There's a breaking change here in the config options -
applicationIDbecomesapplicationIDsand expects an array instead of int.Docs update for the changes here: wtfutil/wtfdocs#38
As mentioned on the original issue, I'm pretty green at golang so feedback is very welcome!