Implement saphostctrl gatherer #71

arbulu89 · 2022-08-05T07:12:50Z

saphostctrl gatherer. Simply returns the output of this commands.

Example:

./trento-agent facts gather --gatherer saphostctrl --argument Ping
INFO[2022-08-05 09:12:00] Starting saphostctrl facts gathering process 
INFO[2022-08-05 09:12:00] Requested saphostctrl facts gathered         
INFO[2022-08-05 09:12:00] Gathered fact for "saphostctrl" with argument "Ping": 
INFO[2022-08-05 09:12:00] {
  "name": "Ping",
  "value": "SUCCESS (  548364 usec)\n",
  "check_id": ""
}

nelsonkopliku

Looks good!

Then, sorry if I bother you with DSL examples 😅 yet I it helps me to keep a connection between the different aspects.

In this case a user would write this piece of yaml

facts:
    - name: my_saphostctrl_ping
      gatherer: saphostctrl
      argument: Ping

LGTM

internal/factsengine/gatherers/saphostctrl.go

CDimonaco

Just the DI thing, than LGTM

dottorblaster

Just a comment, then we can merge

internal/factsengine/gatherers/saphostctrl_test.go

rtorrero

For my side, all relevant things have already been addressed or discussed and agreed to be addressed in next PRs (including the DI).

LGTM

arbulu89 · 2022-08-29T07:32:46Z

@dottorblaster @dottorblaster Finally the requested changes rebased. Could you have a 2nd look?

rtorrero · 2022-12-22T09:20:51Z

@arbulu89 @dottorblaster @CDimonaco @nelsonkopliku I've updated the gatherer to match the latest change in the fact gathering process and improved a bit how the output looks. Take a look at the tests to see if you agree this is usable, and I'll go ahead and write the docs.

Basically, the changes in the output are that we now use a map with parsed output fields instead of the whole output as a single string.

arbulu89

Hey @rtorrero ,
Some changes requested.
Besides that, I would say some paths of the code are not tested (some parsing errors, command error, etc)

internal/factsengine/gatherers/saphostctrl.go

internal/factsengine/gatherers/saphostctrl_test.go

arbulu89 · 2022-12-22T09:44:30Z

internal/factsengine/gatherers/saphostctrl_test.go

+		[]byte("SUCCESS (  543341 usec)\n"), nil)
+	suite.mockExecutor.On("Exec", "/usr/sap/hostctrl/exe/saphostctrl", "-function", "ListInstances").Return(
+		[]byte(" Inst Info : PRD - 00 - myhost-vmhana02 - 753, patch 410, changelist 1908545\n"), nil)
+


I would add other scenario when there is not any instance, to see that it returns an empty map

And we're probably missing a test for a failed ListInstances as well

rtorrero · 2022-12-23T08:00:05Z

@arbulu89 I've addressed some of your concerns and marked them as "resolved" for others, I've come with slightly different approaches but those should also cover your concerns. Please take a look at let me know 👍

nelsonkopliku

Looks good Ruben, thanks!

I noted down a few minor things and then for me we're good.

internal/factsengine/gatherers/saphostctrl_test.go

internal/factsengine/gatherers/saphostctrl.go

nelsonkopliku · 2022-12-23T09:02:25Z

internal/factsengine/gatherers/saphostctrl_test.go

+		[]byte("SUCCESS (  543341 usec)\n"), nil)
+	suite.mockExecutor.On("Exec", "/usr/sap/hostctrl/exe/saphostctrl", "-function", "ListInstances").Return(
+		[]byte(" Inst Info : PRD - 00 - myhost-vmhana02 - 753, patch 410, changelist 1908545\n"), nil)
+


And we're probably missing a test for a failed ListInstances as well

arbulu89

Hey @rtorrero ,
I find the code difficult to follow with so many functions that are not meant to be functions (in my opinion), as they are not reused. And this creates the need to duplicate code many times.
I don't know I have the feeling you aimed to an extreme tailoring for functions. To makes reading difficult, as you need to scroll up and down constantly.

internal/factsengine/gatherers/saphostctrl.go

internal/factsengine/gatherers/saphostctrl_test.go

arbulu89 · 2022-12-23T09:24:07Z

internal/factsengine/gatherers/saphostctrl.go

+			instance["system"] = &entities.FactValueString{Value: fields[1]}
+			instance["instance"] = &entities.FactValueString{Value: fields[2]}
+			instance["hostname"] = &entities.FactValueString{Value: fields[3]}
+			instance["revision"] = &entities.FactValueString{Value: fields[4]}


shouldn't revision, patch and changelist be integers?

arbulu89 · 2022-12-23T09:24:23Z

internal/factsengine/gatherers/saphostctrl.go

+	}
+
+	pingData["status"] = &entities.FactValueString{Value: matches[1]}
+	pingData["elapsed"] = &entities.FactValueString{Value: matches[2]}


Shouldn't elapsed be an integer?

arbulu89 · 2022-12-23T09:25:49Z

internal/factsengine/gatherers/saphostctrl.go

+	}
+	SapHostCtrlUnsupportedFunction = entities.FactGatheringError{
+		Type:    "saphostctrl-webmethod-error",
+		Message: "requested webmethod not whitelisted",


I would rephrase this to requested webmethod is not supported

arbulu89 · 2022-12-23T09:27:02Z

internal/factsengine/gatherers/saphostctrl_test.go

+func (suite *SapHostCtrlTestSuite) TestSapHostCtrlGatherError() {
+	suite.mockExecutor.On("Exec", "/usr/sap/hostctrl/exe/saphostctrl", "-function", "Ping").Return(
+		[]byte("FAILURE\n"), nil)
+	suite.mockExecutor.On("Exec", "/usr/sap/hostctrl/exe/saphostctrl", "-function", "Pong").Return(


What about an scenario where the command fails.
Here we have a

parsing error

unsupported command

So we miss the command error test

arbulu89

@rtorrero
Super perfect from my side! I cannot approve since I opened the PR XD

Old review

arbulu89 added the enhancement New feature or request label Aug 5, 2022

arbulu89 requested review from dottorblaster, fabriziosestito, rtorrero, nelsonkopliku and CDimonaco August 5, 2022 07:12

nelsonkopliku approved these changes Aug 9, 2022

View reviewed changes

arbulu89 force-pushed the saphostctrl-gatherer branch 2 times, most recently from d7d95c6 to 7f0308f Compare August 9, 2022 09:35

CDimonaco reviewed Aug 9, 2022

View reviewed changes

internal/factsengine/gatherers/saphostctrl.go Outdated Show resolved Hide resolved

CDimonaco previously requested changes Aug 9, 2022

View reviewed changes

dottorblaster previously requested changes Aug 9, 2022

View reviewed changes

internal/factsengine/gatherers/saphostctrl_test.go Outdated Show resolved Hide resolved

arbulu89 force-pushed the saphostctrl-gatherer branch from f08bd33 to 7f0308f Compare August 9, 2022 12:57

rtorrero approved these changes Aug 11, 2022

View reviewed changes

nelsonkopliku self-requested a review August 22, 2022 07:47

arbulu89 force-pushed the saphostctrl-gatherer branch from 7f0308f to 10b4b15 Compare August 29, 2022 07:32

arbulu89 requested review from CDimonaco and dottorblaster August 29, 2022 07:32

rtorrero force-pushed the saphostctrl-gatherer branch from 10b4b15 to 74f90c7 Compare December 20, 2022 16:26

rtorrero force-pushed the saphostctrl-gatherer branch from 1129bcf to 7720a66 Compare December 22, 2022 09:21

arbulu89 and others added 3 commits December 22, 2022 09:22

Implement saphostctrl gatherer

02275ec

Update saphostctrl gatherer to work with the latest changes

07e28ca

Add whitelisting of Ping and ListInstances command

1811273

arbulu89 commented Dec 22, 2022

View reviewed changes

Improve code & address comments

b143356

rtorrero force-pushed the saphostctrl-gatherer branch 2 times, most recently from 2603ee1 to e9f0e63 Compare December 22, 2022 17:23

Improve readability & modularity

e9f0e63

rtorrero added 3 commits December 22, 2022 17:55

Add support to parse FAILED ping

c6b86ef

Add more tests

cef9b12

Address linter warnings

aefa89a

nelsonkopliku requested changes Dec 23, 2022

View reviewed changes

arbulu89 commented Dec 23, 2022

View reviewed changes

Reduce function granularity

a8e7b28

nelsonkopliku force-pushed the saphostctrl-gatherer branch from 393b8d0 to a8e7b28 Compare December 23, 2022 10:36

Further addressing of PR comments

4e2a149

arbulu89 commented Dec 23, 2022

View reviewed changes

rtorrero requested a review from nelsonkopliku December 23, 2022 13:02

nelsonkopliku approved these changes Dec 23, 2022

View reviewed changes

rtorrero merged commit 3798072 into main Dec 23, 2022

rtorrero deleted the saphostctrl-gatherer branch December 23, 2022 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement saphostctrl gatherer #71

Implement saphostctrl gatherer #71

arbulu89 commented Aug 5, 2022

nelsonkopliku left a comment

CDimonaco left a comment •

edited

Loading

dottorblaster left a comment

rtorrero left a comment

arbulu89 commented Aug 29, 2022

rtorrero commented Dec 22, 2022 •

edited

Loading

arbulu89 left a comment

arbulu89 Dec 22, 2022

nelsonkopliku Dec 23, 2022

rtorrero commented Dec 23, 2022

nelsonkopliku left a comment

nelsonkopliku Dec 23, 2022

arbulu89 left a comment

arbulu89 Dec 23, 2022

arbulu89 Dec 23, 2022

arbulu89 Dec 23, 2022

arbulu89 Dec 23, 2022

arbulu89 left a comment

Implement saphostctrl gatherer #71

Implement saphostctrl gatherer #71

Conversation

arbulu89 commented Aug 5, 2022

nelsonkopliku left a comment

Choose a reason for hiding this comment

CDimonaco left a comment • edited Loading

Choose a reason for hiding this comment

dottorblaster left a comment

Choose a reason for hiding this comment

rtorrero left a comment

Choose a reason for hiding this comment

arbulu89 commented Aug 29, 2022

rtorrero commented Dec 22, 2022 • edited Loading

arbulu89 left a comment

Choose a reason for hiding this comment

arbulu89 Dec 22, 2022

Choose a reason for hiding this comment

nelsonkopliku Dec 23, 2022

Choose a reason for hiding this comment

rtorrero commented Dec 23, 2022

nelsonkopliku left a comment

Choose a reason for hiding this comment

nelsonkopliku Dec 23, 2022

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

arbulu89 Dec 23, 2022

Choose a reason for hiding this comment

arbulu89 Dec 23, 2022

Choose a reason for hiding this comment

arbulu89 Dec 23, 2022

Choose a reason for hiding this comment

arbulu89 Dec 23, 2022

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

CDimonaco left a comment •

edited

Loading

rtorrero commented Dec 22, 2022 •

edited

Loading