-
Notifications
You must be signed in to change notification settings - Fork 572
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: in some cases, try to use pom info to guess name and version to top level jar #2080
Changes from 6 commits
f347fb0
44df88f
714d208
09bdd84
37337b5
1f37909
878b5a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
package integration | ||
|
||
import ( | ||
"strings" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
|
||
"github.com/anchore/syft/syft/pkg" | ||
"github.com/anchore/syft/syft/source" | ||
) | ||
|
||
func TestWarCatalogedCorrectlyIfRenamed(t *testing.T) { | ||
// install hudson-war@2.2.1 and renames the file to `/hudson.war` | ||
// TODO: is this better expressed in syft/pkg/cataloger/java/archive_parser_test.go? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, I'm not against having this integration test at all, so consider this comment non-blocking, however, testing specific business logic should be pushed as low as possible test-level-wise (unit is preferred for these kinds of assertions). We do have some regression tests at the integration level to test against the specific asset where we found the regression, so keeping this test here is ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion! I think I'll leave this here for now; it was reported as an end to end regression, and the parser was still finding the correct package, it was just also finding an incorrect extra package, so the end-to-end flow seems a good place of the test. I'll push a commit to remove the TODO. |
||
sbom, _ := catalogFixtureImage(t, "image-java-virtualpath-regression", source.SquashedScope, nil) | ||
|
||
badPURL := "pkg:maven/hudson/hudson@2.2.1" | ||
goodPURL := "pkg:maven/org.jvnet.hudson.main/hudson-war@2.2.1" | ||
foundCorrectPackage := false | ||
badVirtualPath := "/hudson.war:org.jvnet.hudson.main:hudson-war" | ||
goodVirtualPath := "/hudson.war" | ||
for _, p := range sbom.Artifacts.Packages.Sorted() { | ||
if p.Type == pkg.JavaPkg && strings.Contains(p.Name, "hudson") { | ||
assert.NotEqual(t, badPURL, p.PURL, "must not find bad purl %q", badPURL) | ||
virtPath := "" | ||
if meta, ok := p.Metadata.(pkg.JavaMetadata); ok { | ||
virtPath = meta.VirtualPath | ||
if p.PURL == goodPURL && virtPath == goodVirtualPath { | ||
foundCorrectPackage = true | ||
} | ||
} | ||
assert.NotEqual(t, badVirtualPath, virtPath, "must not find bad virtual path %q", badVirtualPath) | ||
} | ||
} | ||
assert.True(t, foundCorrectPackage, "must find correct package, but did not") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
FROM alpine:latest | ||
|
||
RUN wget https://repo1.maven.org/maven2/org/jvnet/hudson/main/hudson-war/2.2.1/hudson-war-2.2.1.war | ||
|
||
RUN mv hudson-war-2.2.1.war hudson.war | ||
|
||
|
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.
or maybe rename
selectName
to something more specific likeselectNameFromManifest
(same for the version function) -- the upside is that you can write aselectNameAndVersion
wrapper that is called here and pushes some of this lower level logic to another function, and the existing tests for selectName don't really need to get modified from a logic sense.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.
Thanks for the suggestion! I think I'll do this as a follow up.