-
Notifications
You must be signed in to change notification settings - Fork 590
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
feat: add cyclonedx schema version selection #2123
Changes from 8 commits
b9988df
eb009e8
12355d8
beffca2
1f62e19
9aec01d
0e2ddc4
8cffa74
0f80acc
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,34 @@ | ||
package cyclonedxjson | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/CycloneDX/cyclonedx-go" | ||
) | ||
|
||
func TestFormatVersions(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
expectedVersion string | ||
}{ | ||
{ | ||
|
||
"cyclonedx-json should default to v1.4", | ||
cyclonedx.SpecVersion1_4.String(), | ||
}, | ||
} | ||
|
||
for _, c := range tests { | ||
c := c | ||
t.Run(c.name, func(t *testing.T) { | ||
sbomFormat := Format() | ||
if sbomFormat.ID() != ID { | ||
t.Errorf("expected ID %q, got %q", ID, sbomFormat.ID()) | ||
} | ||
|
||
if sbomFormat.Version() != c.expectedVersion { | ||
t.Errorf("expected version %q, got %q", c.expectedVersion, sbomFormat.Version()) | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,17 +26,27 @@ import ( | |
func Formats() []sbom.Format { | ||
return []sbom.Format{ | ||
syftjson.Format(), | ||
cyclonedxxml.Format(), | ||
cyclonedxjson.Format(), | ||
github.Format(), | ||
table.Format(), | ||
text.Format(), | ||
template.Format(), | ||
cyclonedxxml.Format1_0(), | ||
cyclonedxxml.Format1_1(), | ||
cyclonedxxml.Format1_2(), | ||
cyclonedxxml.Format1_3(), | ||
cyclonedxxml.Format1_4(), | ||
cyclonedxxml.Format1_5(), | ||
cyclonedxjson.Format1_0(), | ||
cyclonedxjson.Format1_1(), | ||
cyclonedxjson.Format1_2(), | ||
cyclonedxjson.Format1_3(), | ||
cyclonedxjson.Format1_4(), | ||
cyclonedxjson.Format1_5(), | ||
spdxtagvalue.Format2_1(), | ||
spdxtagvalue.Format2_2(), | ||
spdxtagvalue.Format2_3(), | ||
spdxjson.Format2_2(), | ||
spdxjson.Format2_3(), | ||
table.Format(), | ||
text.Format(), | ||
template.Format(), | ||
} | ||
} | ||
|
||
|
@@ -55,7 +65,7 @@ func Identify(by []byte) sbom.Format { | |
|
||
// ByName accepts a name@version string, such as: | ||
// | ||
// spdx-json@2.1 or cyclonedx@2 | ||
// spdx-json@2.1 or cyclonedx@1.5 | ||
func ByName(name string) sbom.Format { | ||
parts := strings.SplitN(name, "@", 2) | ||
version := sbom.AnyVersion | ||
|
@@ -71,6 +81,16 @@ func ByNameAndVersion(name string, version string) sbom.Format { | |
for _, f := range Formats() { | ||
for _, n := range f.IDs() { | ||
if cleanFormatName(string(n)) == name && versionMatches(f.Version(), version) { | ||
// if the version is not specified and the format is cyclonedx, then we want to return the most recent version up to 1.4 | ||
// If more aliases like cdx are added this will not catch those - we want to eventually provide a way for | ||
// formats to inform this function what their default version is | ||
// TODO: remove this check when 1.5 is stable or default formats are designed. PR below should be merged. | ||
// https://github.com/CycloneDX/cyclonedx-go/pull/90 | ||
if version == sbom.AnyVersion && strings.Contains(string(n), "cyclonedx") { | ||
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. This is looking for the string At the same time, I feel that this format object is about to be rethought anyway. I think there are strong signs to split up for encoding vs decoding use cases, which means trying to settle this detail in this PR wouldn't be worth it. I think it's worth at least adding a comment here about the limitations of this approach and a TODO to refactor later. 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. also, how does ByName work for spdx for the latest version (I didn't read up on that yet)? 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.
^ Yea that sounds good to me
I had tried just setting up a default version for each format rather than The comment for this line should be below it I can move above and add a TODO:
For SPDX if you pass in 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. I would probably like to add a 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. I can follow this PR up with a stab at that. I really want to keep this PR small and focused on integrating the released cyclonedx library. Other changes on how we handle formats, their defaults or how the latest is selected can be their own change set. The reason this line had to be included was because we don't necessarily want to be on latest for cyclonedx at this moment. Rather than rewrite ByNameAndVersion into a totally new thing I thought a simple edge case with comment would be the correct change here 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. This check should probably be |
||
if f.Version() == "1.5" { | ||
continue | ||
} | ||
} | ||
if mostRecentFormat == nil || f.Version() > mostRecentFormat.Version() { | ||
mostRecentFormat = f | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package syftjson | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/anchore/syft/internal" | ||
) | ||
|
||
func TestFormat(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
version string | ||
}{ | ||
{ | ||
name: "default version should use latest internal version", | ||
version: "", | ||
}, | ||
} | ||
|
||
for _, c := range tests { | ||
c := c | ||
t.Run(c.name, func(t *testing.T) { | ||
sbomFormat := Format() | ||
if sbomFormat.ID() != ID { | ||
t.Errorf("expected ID %q, got %q", ID, sbomFormat.ID()) | ||
} | ||
|
||
if sbomFormat.Version() != internal.JSONSchemaVersion { | ||
t.Errorf("expected version %q, got %q", c.version, sbomFormat.Version()) | ||
} | ||
}) | ||
} | ||
} |
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.
It might be nice to programmatically build these formats, something like this could live in the
common/cyclonedxhelpers
package:This way there only needs to be a single
encodeCycloneDX()
function and adding new versions is just updating the list, which would update both XML and JSON variants. E.g. the XML variant would have: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.
This is a good suggestion, but I think we wanted to save this kind of work for when we go back and take a look at Formats before syft v1.0. The purpose of this PR was to just keep this as close to the same pattern as we have with SPDX. Getting creative here and trying to build these programmatically (different from the current SPDX pattern) would eventually get swallowed by that new work.
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.
agreed to the both of you -- it's a good suggestion, but it would be better to be consistent with the current pattern first, then refactor to "the next" pattern.