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

fix upstream match for linux-.*-headers-.* #2320

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Conversation

barnuri
Copy link
Contributor

@barnuri barnuri commented Dec 12, 2024

a fix for this PR, the upstream of linux-.-headers-. is linux-aws, linux-azure etc
#2295

Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

We should figure out how to add some tests for this, maybe something in the test/cli package?

@barnuri
Copy link
Contributor Author

barnuri commented Dec 12, 2024

We should figure out how to add some tests for this, maybe something in the test/cli package?

i think its very small change and no so familiar with your tests
if you really think its necessary please give me details how to do so 😄

@barnuri barnuri requested a review from kzantow December 14, 2024 19:33
@barnuri
Copy link
Contributor Author

barnuri commented Dec 19, 2024

@kzantow ?

@tomersein
Copy link
Contributor

hi! any updates? I've tried to look and see if it is easy to add tests here but I didn't find any simple way (and its looks like a small change which can reduce lots of FP)

@kzantow
Copy link
Contributor

kzantow commented Jan 2, 2025

I still think we should add a test -- a small change like this could easily regress without one. How about updating these "ignore" tests?

@tomersein
Copy link
Contributor

I've noticed the UT is not protecting as expected the code, since the ignore rules are inside the root:

var ignoreLinuxKernelHeaders = []match.IgnoreRule{
	{Package: match.IgnoreRulePackage{Name: "kernel-headers", UpstreamName: "kernel", Type: string(syftPkg.RpmPkg)}, MatchType: match.ExactIndirectMatch},
	{Package: match.IgnoreRulePackage{Name: "linux(-.*)?-headers-.*", UpstreamName: "linux.*", Type: string(syftPkg.DebPkg)}, MatchType: match.ExactIndirectMatch},
	{Package: match.IgnoreRulePackage{Name: "linux-libc-dev", UpstreamName: "linux", Type: string(syftPkg.DebPkg)}, MatchType: match.ExactIndirectMatch},
}

but they are manually built in the ignore_test, so in case of a change it doesn't detect it.

I suggest moving it or refactor the tests.. not sure how.
@kzantow

github-actions and others added 2 commits January 5, 2025 16:21
@barnuri
Copy link
Contributor Author

barnuri commented Jan 14, 2025

@kzantow hi there is any update of this review ?

Comment on lines 218 to 221
pattern, err := packageNameRegex(name)
if err != nil {
continue
}
Copy link
Contributor

@kzantow kzantow Jan 15, 2025

Choose a reason for hiding this comment

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

I think this should be moved outside of the returned func, the same way ifPackageNameApplies checks the regex once and returns an "always false" func; and we should add debug logging for failures in both places e.g.:

func ifUpstreamPackageNameApplies(name string) ignoreCondition {
	pattern, err := packageNameRegex(name)
	if err != nil {
		log.WithFields("name", name, "error", err).Debug("unable to parse name expression")
		return func(Match) bool { return false }
	}
	return func(match Match) bool {
		for _, upstream := range match.Package.Upstreams {
			if pattern.MatchString(upstream.Name) {
				return true
			}
		}
		return false
	}
}

Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

This looks great -- one small tweak requested

@tomersein
Copy link
Contributor

hi @kzantow , pushed a fix :)

Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @barnuri and @tomersein !

@kzantow kzantow merged commit 5812bb8 into anchore:main Jan 15, 2025
10 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.

3 participants