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

Refactor so that testdata samples are in their own namespace #110

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

tstromberg
Copy link
Collaborator

Related to #75

This was particularly nice as we were able to move tests into their own namespace.

We may move bincapz.go to pkg/bincapz, but I'm not sure yet.

@cpanato
Copy link
Member

cpanato commented Apr 11, 2024

need a rebase

vaikas
vaikas previously approved these changes Apr 11, 2024
pkg/report/report.go Outdated Show resolved Hide resolved
@@ -297,16 +297,20 @@ func Generate(ctx context.Context, path string, mrs yara.MatchRules, ignoreTags
overallRiskScore := 0
riskCounts := map[int]int{}

for _, m := range mrs {
for x, m := range mrs {
clog.InfoContextf(ctx, "yara match[%d]: %+v", x, m)
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but a better approach might be to take advantage of the structured logging fields, so we'd set the MatchRule once, and we don't have to keep logging it. This makes it easier to search for logs in some places because the "context" fields are set in the right context, and you don't need to keep tacking that in to each log message.
So, something like this here:
mrLog := clog.FromContext(ctx).With(slog.String("matchrule", m.Namespace))

And then below you could just do:
mrLog.Infof("dropping (risk %d is too low)", risk)
and
mrLog.Infof("key=%s", key)

so you don't have to keep logging the m.Namespace in different places. But just more FYI in case you weren't aware of it.

Copy link
Member

Choose a reason for hiding this comment

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

In the other PR I did try to use it in places, but just wanted to take the first pass of moving to structured logging, I'm sure we can clean up the logging more intentionally later on.

@tstromberg tstromberg enabled auto-merge (squash) April 11, 2024 12:27
@tstromberg tstromberg merged commit 2934b76 into chainguard-dev:main Apr 11, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants