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

Saptune gatherer #256

Merged
merged 5 commits into from
Sep 27, 2023
Merged

Saptune gatherer #256

merged 5 commits into from
Sep 27, 2023

Conversation

rtorrero
Copy link
Contributor

@rtorrero rtorrero commented Sep 20, 2023

This PR adds a new Saptune gatherer using the same saptune core code as the #253 (saptune_discovery). The whitelisted saptune arguments are the following:

var whitelistedArguments = map[string][]string{
	"status":          {"status", "--non-compliance-check"},
	"solution-verify": {"solution", "verify"},
	"solution-list":   {"solution", "list"},
	"note-verify":     {"note", "verify"},
	"note-list":       {"note", "list"},
}

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.

@rtorrero Some few things.
Besides that, i don't still see any "caching" system.
I mean, we must only run the commands for each argument once, so you need to check if they were executed, and store the output

internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
@rtorrero rtorrero force-pushed the saptune-gatherer branch 2 times, most recently from 31239f5 to 41bf2a9 Compare September 26, 2023 11:25
@rtorrero rtorrero marked this pull request as ready for review September 26, 2023 11:25
internal/factsengine/gatherers/gatherer.go Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for purity we could remove this first empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it because I noticed the output of saptune adds it, but it shouldn't make any difference

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.

Good!
Once we have this minor things fixed we are ready to merge.
You can start working in the documentation of the gatherer in wanda if you want

internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/saptune.go Outdated Show resolved Hide resolved
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.

Thank you @rtorrero
Ready to merge from my side.
PD: added last nitpick comment if you want to have a look 🙈

@@ -70,22 +70,21 @@ func (s *SaptuneGatherer) Gather(factsRequests []entities.FactRequest) ([]entiti
facts := []entities.Fact{}
log.Infof("Starting %s facts gathering process", SaptuneGathererName)
saptuneRetriever, err := saptune.NewSaptune(s.executor)

if err != nil {
return facts, &SaptuneNotInstalled
Copy link
Contributor

Choose a reason for hiding this comment

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

We could return nil, &SaptuneNotInstalled as well (the same for the next)
And even SaptuneNotInstalled.Wrap(err.Error()) in this case, as we have the rpm command error

@rtorrero rtorrero merged commit 72c8765 into main Sep 27, 2023
10 checks passed
@rtorrero rtorrero deleted the saptune-gatherer branch September 27, 2023 12:38
@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
Development

Successfully merging this pull request may close these issues.

3 participants