Identify SAP diagnostics agent #55
Conversation
arbulu89
requested review from
dottorblaster,
fabriziosestito,
rtorrero and
CDimonaco
| } | ||
|
|
||
| var instanceType int | ||
| for _, instance := range sapControl.Instances { |
There was a problem hiding this comment.
As I understand here, we loop through the instances but instanceType will be always the last of instances that has sapLocalhost, because we don't break the loop, it's an expected behavior? Maybe if we are only interested in the "first one", or "at least one" of the instances is sapLocalhost we can return into the cases of the switch without looping.
Because if in the instances no one is the sapLocalhost instanceType will be the zero value of int so zero, maybe if this is expected we can initialize the variable not to int but to the appropriate "enum value" like "Application" "Database" etc..
There was a problem hiding this comment.
Maybe if the "enum values" are a type alias and not just int, we can refactor the signature of the method in order to return (AliasType, error), so the caller could infer that the value should be one of the aliased values of int defined before
There was a problem hiding this comment.
Good idea.
Refactored here: 6af485f
Besides that, I have removed some legacy dead code that I found...
We will have more most probably...
Cherry-pick missing commit from legacy trento: trento-project/trento@3467e77
I have done some additional changes to adapt the code, as we already were on some conflicts. But the code in essence is the same, so a deep review shouldn't be needed.
After this, some small changes in the web side are needed too.