-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add saptune discovery #253
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
9712545
to
1d87676
Compare
c5130db
to
11d0755
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rtorrero
Some feedback.
Consider adding a IsJsonSupported
field. I think it will come really useful in the gatherers as well, as we can skip running command functions since the beginning
internal/core/saptune/saptune.go
Outdated
return nil, ErrUnsupportedSaptuneVer | ||
} | ||
|
||
log.Info("Requesting Saptune status...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
e9d4726
to
5f659c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Almost ready to merge, once we fix the linting things mostly
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could name this expectedSaptune
. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rtorrero green light from my side.
It would be nice to run some manual integration test between agent and web to see that it works.
(remember that there is a pending PR in web which fixes the policy)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 status
and expose this through the collector