-
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
Disp work gatherer #281
Disp work gatherer #281
Conversation
3fd8368
to
f342042
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.
For me the code looks good, I really like the named groups expansion
Just some comments to understand better the usecase
|
||
result := fillRegexpGroups(string(dispWorkOutput)) | ||
|
||
dispWorkMap[sid] = dispWorkData{ |
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.
What happens if we have no matches on the regexp, for example something could be missing or could be empty, we allow that?
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.
In that cases the map is filled with empty strings. The FindAllStringSubmatch
function returns a value, even an empty one for group, so combined with the SubexpNames
you can fill all the groups, at least with empty values.
Actually, I added a test to cover that case XD
if err != nil { | ||
gatheringError := DispWorkCommandError.Wrap(err.Error()) | ||
log.Error(gatheringError) | ||
dispWorkMap[sid] = dispWorkData{} // fill with empty data |
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 don't want to abort on something like that?
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 thought about that, but the fact that we might have multiple sap installations doesn't make it a good idea in my opinion.
If you have NWP
and NWD
installed, and one fails, you at least have the result for the other (and the failed one filled with empty strings, that at the end, would make the rhai
test fail
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.
@arbulu89 LGTM, only thing I spotted is that you didn't add a test for the command-error case, should we add it?
The |
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 lgtm :D
disp+work
gatherer implementation. It runs thedisp+work
utility for eachsidadm
user, and parses the output.By now, as requested, it only returns the
kernel_release
,compilation_mode
andpatch_number
.Everything is parsed using one regexp pattern. Adding new fields should be straightforward.
The
fillRegexpGroups
function is inspired in theregroup
package: https://github.com/oriser/regroup#named-groups-mapI have created the internal
dispWorkData
struct. It is true that i could've simply use the map, without the json transformation, but I see that having the struct is more "strict" and informativeOutput: