Add saptune discovery #253
Conversation
There was a problem hiding this comment.
Hey @rtorrero ,
Some initial comments.
Besides that, I wonder how this code could be reusable in a gatherer. I would maybe expect some RunSaptuneCommand(cmd, format), where you pass the command you want to execute and maybe the format.
rtorrero
force-pushed
the
add-saptune-discovery
branch
from
9712545
to
1d87676
Compare
rtorrero
force-pushed
the
add-saptune-discovery
branch
from
c5130db
to
11d0755
Compare
internal/core/saptune/saptune.go
Outdated
| return nil, ErrUnsupportedSaptuneVer | ||
| } | ||
|
|
||
| log.Info("Requesting Saptune status...") |
There was a problem hiding this comment.
These initial log messages are outdated, right?
internal/core/saptune/saptune.go
Outdated
| } | ||
|
|
||
| func (s *Saptune) RunCommand(args ...string) ([]byte, error) { | ||
| if !isSaptuneVersionSupported(s.Version) { |
There was a problem hiding this comment.
Are we sure we want to run this for every command?
If we had the isJsonSupported field, we could avoid running this.
Actually, the code owner (us) could even avoid using RunCommand, as we know that json is not supported before hand
internal/core/saptune/saptune.go
Outdated
| ) | ||
|
|
||
| type Saptune struct { | ||
| Version string `json:"version"` |
There was a problem hiding this comment.
I don't think we need the json annotation.
This struct is internal, not meant to be marshalled
internal/discovery/saptune.go
Outdated
|
|
||
| func (d SaptuneDiscovery) Discover() (string, error) { | ||
| saptuneRetriever, err := saptune.NewSaptune(utils.Executor{}) | ||
| if err != nil { |
There was a problem hiding this comment.
We cannot do that. Even if we get some error here, we need to send a payload, with the PackageVersion: null/empty string, SaptuneInstalled: false and Status: nil
rtorrero
force-pushed
the
add-saptune-discovery
branch
from
e9d4726
to
5f659c3
Compare
internal/core/saptune/saptune.go
Outdated
| func NewSaptune(commandExecutor utils.CommandExecutor) (Saptune, error) { | ||
| saptuneVersion, err := getSaptuneVersion(commandExecutor) | ||
| if err != nil { | ||
| return Saptune{Version: ""}, err |
There was a problem hiding this comment.
I think you could simply return Saptune{}
internal/core/saptune/saptune.go
Outdated
| return Saptune{Version: ""}, err | ||
| } | ||
|
|
||
| return Saptune{Version: saptuneVersion, executor: commandExecutor, IsJSONSupported: isSaptuneVersionSupported(saptuneVersion)}, nil |
There was a problem hiding this comment.
Maybe better to store the result in a variable, just to improve readability
|
|
||
| saptuneRetriever, err := NewSaptune(mockCommand) | ||
|
|
||
| expectedDetails := Saptune{ |
There was a problem hiding this comment.
Maybe we could name this expectedSaptune. Wdyt?
internal/core/saptune/saptune.go
Outdated
| } | ||
|
|
||
| prependedArgs := append([]string{"--format", "json"}, args...) | ||
| log.Infof("Running saptune command: saptune %v", args) |
There was a problem hiding this comment.
We could reuse the main RunCommand once we have the new args.
Not a big deal
return s.RunCommand(prependedArgs)
internal/discovery/saptune.go
Outdated
| default: | ||
| saptuneData, _ := saptuneRetriever.RunCommandJSON("status") | ||
| unmarshalled := make(map[string]interface{}) | ||
| _ = json.Unmarshal(saptuneData, &unmarshalled) |
There was a problem hiding this comment.
I think we should capture the error and exit if there is any.
Just for safetiness.
The status should always return a json, but just in case...
Add initial implementation of the
saptune --format json statusand expose this through the collector