sysctl gatherer #268
sysctl gatherer #268
Conversation
rtorrero
force-pushed
the
add-sysctl-gatherer
branch
from
520e611 to
3e767e1
Compare
There was a problem hiding this comment.
We need to add the gatherer to the list of standard gatherers
The envparse usage comment is a personal opinion. I think that it would simplify the code. Specially, because i don't expect the gatherer being called with "intermediate" values
PD: Please, test things manually as well, to see that it does what we expect
|
|
||
| func (s *SysctlGatherer) Gather(factsRequests []entities.FactRequest) ([]entities.Fact, error) { | ||
| facts := []entities.Fact{} | ||
| log.Infof("Starting sysctl facts gathering process") |
There was a problem hiding this comment.
We could use the string interpolation with SysctlGathererName as we do with many others.
Small change, but it would help us if we for some reason change the gatherer name, to not forget this details
| facts = append(facts, fact) | ||
| } | ||
|
|
||
| log.Infof("Requested %s facts gathered", CorosyncCmapCtlGathererName) |
There was a problem hiding this comment.
Forgot to change this I guess hehe
| return facts, nil | ||
| } | ||
|
|
||
| func sysctlOutputToMap(output []byte) *entities.FactValueMap { |
There was a problem hiding this comment.
Could we try to use envparse.Parse(output) maybe?
At the end the output looks like an .env file format.
I know that this wouldn't create a map of maps, but I don't think we would need such a thing in this case. I expect the user putting the whole needed line to get individual values
I would try to change, you have the tests there to confirm that everything went right
PD: I know that this is based in the corosynccmapctl gatherer, but i think we could make it simpler this way
There was a problem hiding this comment.
Indeed it's based on the corosync one, I saw a comment in confluence mentioning that we might want to expose sysctl -a (with no arguments) as a map, so I used the same approach. In the end apparently we don't need this but if we do in the future, the current approach would be more powerful. In any case, no hard opinions on this.
There was a problem hiding this comment.
Anyway, do we need all of this switch code with the SplitN?
I think the corosynccmapctl code is cleaner, and does exactly the same (99% the same)
parts := strings.Split(line, " =")
key := parts[0]
value := parts[1]
f58b533 to
121affa
Compare
| cursor := outputMap | ||
| pathComponents := strings.Split(key, ".") | ||
|
|
||
| for i, component := range pathComponents { |
There was a problem hiding this comment.
I still think that the code in corosynccmapctl is simpler, with less if/else branching.
Anyway, I guess it works fine
| for _, line := range strings.Split(string(output), "\n") { | ||
| parts := strings.SplitN(line, "=", 2) | ||
| if len(line) == 0 || len(parts) != 2 { | ||
| continue |
There was a problem hiding this comment.
I don't know if we shouldn't raise an error in this case, as we would get a malformed output
There was a problem hiding this comment.
Some sort of decoding error as we do in other gatherers
This PR adds a
sysctl. Note: Right now, it will fail if no argument is provided but if we want, we could allow zero arguments to return the whole sysctl map.