Skip to content
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

Sapservices gatherer #282

Merged
merged 6 commits into from
Oct 26, 2023
Merged

Sapservices gatherer #282

merged 6 commits into from
Oct 26, 2023

Conversation

CDimonaco
Copy link
Member

Gatherer for sapservices file, by default /usr/sap/sapservices.

Return each entry of sapservices file as a map, with the kind of startup system used, systemd or sapservices

@CDimonaco CDimonaco self-assigned this Oct 19, 2023
@CDimonaco CDimonaco added the go Pull requests that update Go code label Oct 19, 2023
@CDimonaco CDimonaco force-pushed the sapservices-gatherer branch 3 times, most recently from 2c67751 to 9b60482 Compare October 19, 2023 15:04
@CDimonaco CDimonaco force-pushed the sapservices-gatherer branch from 9b60482 to 2871a16 Compare October 19, 2023 15:42
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code logic looks totally correct.
I'm wondering if we should do some integrity check on the line contents. I mean, to check if the line with systemctl or sapstartsrv follows the correct structure

internal/factsengine/gatherers/sapservices.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/sapservices.go Show resolved Hide resolved
internal/factsengine/gatherers/sapservices.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/sapservices.go Show resolved Hide resolved
internal/factsengine/gatherers/sapservices.go Outdated Show resolved Hide resolved
@CDimonaco CDimonaco requested a review from rtorrero October 19, 2023 16:31
@CDimonaco CDimonaco marked this pull request as ready for review October 19, 2023 16:31
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to add the gatherer to the list of available gatherers
Maybe you can "mix" the 2 success tests to avoid the "duplicated" warning message

internal/factsengine/gatherers/sapservices.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CDimonaco, looks good to me overall but the names of the tests and content of the errors need some fixing

internal/factsengine/gatherers/sapservices.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/sapservices_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as we had a chat and its now clear what I meant, feel free to fix and merge when ready 👍

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Some cosmetic suggestions.
At least fix the alphabetical ordering of the getherer.go file.
About the linting error, I guess you will need to put some nolint flag there, don't merge with the error please

internal/factsengine/gatherers/gatherer.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/sapservices.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/sapservices.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/sapservices.go Outdated Show resolved Hide resolved
@CDimonaco CDimonaco force-pushed the sapservices-gatherer branch 2 times, most recently from 69c25f8 to 49380ee Compare October 26, 2023 12:02
@CDimonaco CDimonaco merged commit ff79cc9 into main Oct 26, 2023
9 checks passed
@CDimonaco CDimonaco deleted the sapservices-gatherer branch October 26, 2023 12:47
@CDimonaco CDimonaco added the enhancement New feature or request label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request go Pull requests that update Go code
Development

Successfully merging this pull request may close these issues.

3 participants