Conversation
a88f68c to
18d9793
Compare
18d9793 to
e3c491c
Compare
e3c491c to
b0415ad
Compare
b0415ad to
425747b
Compare
425747b to
89ceb7a
Compare
89ceb7a to
cf39866
Compare
attiasas
left a comment
There was a problem hiding this comment.
Nice Job, please take a look at my comments.. In addition:
- This change makes the command behave a bit differently when passing the
tableformat, not enriching, only printing... - Add a description to your PR that reflects your changes.
- Resolve conflict and align code against the updated target branch
dev(notmain...) - Try to add integration tests to
enrich_test.go
| f, err = outputFormat.ParseOutputFormat(format, outputFormat.All) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
No need for this function.
Use outputFormat.ParseOutputFormat(format,{options...}) directly.
You should register a flag with the default value at cli/docs/flags.go see how the CurationOutput option is defined/used, you have a similar case
There was a problem hiding this comment.
Actually this function was used before by some of the commands here.
But it was located in jfrog-cli-core but has been deprecated there.
So I move it back here to avoid changing to many thing.
An effort was made make the declaration of OutputFormat easy an common to all plugin, check the section 'Define output formats' in https://github.com/jfrog/jfrog-cli-core/blob/master/plugins/README.md.
With this new mechanism no need to declare the format flag it is done by the framework, and it can be retrieved from the context ctx.GetOutputFormat()
There was a problem hiding this comment.
After rebasing over dev, I saw that you've already get rid of the deprecated function.
Beware that function used to return Table as the default and ParseOutputFormat does not.
Hope it did not break anything.
cc @attiasas
| Description: enrichDocs.GetDescription(), | ||
| Arguments: enrichDocs.GetArguments(), | ||
| Category: securityCategory, | ||
| SupportedFormats: []outputFormat.OutputFormat{outputFormat.Json, outputFormat.Table}, |
There was a problem hiding this comment.
I can see this option has been added recently to the core logic.
Currently CLI uses the plugins Flags attribute. as mentioned in other comment...
In addition seems like the current implementation is a mix:
You have the GetOutputFormat func you added with table as default but the core logic added DefaultFormat attribute as well as the SupportedFormats, why not use that?
I prefer the way the current repository handles the flags, when migrating we should probably migrate all and not having a mix
There was a problem hiding this comment.
The enrich command already used c.GetOutputFormat()
I did not want to touch the others ones as this is just about the one without format.
| if enrichCmd.outputFormat == coreformat.Table { | ||
| if err = enrichCmd.printVulnerabilitiesTable(scanResults, os.Stdout); err != nil { | ||
| return | ||
| } | ||
| log.Info("Enrich process completed successfully.") | ||
| return | ||
| } |
There was a problem hiding this comment.
This is not enriching the input files
jf sbom-enrich - Enrich sbom format JSON located on the local file-system with Xray.
This option is only printing the Xray results in a table format. IDK if that is intended.
There was a problem hiding this comment.
Not really, So I guess this format is not relevant here... Removing it
| Description: enrichDocs.GetDescription(), | ||
| Arguments: enrichDocs.GetArguments(), | ||
| Category: securityCategory, | ||
| SupportedFormats: []outputFormat.OutputFormat{outputFormat.Json, outputFormat.Table}, |
There was a problem hiding this comment.
In addition, the current command also supports XML output format.
I can't see the option here so we need to add it to avoid regression
There was a problem hiding this comment.
Sure we can add it.
It is the default behavior BTW if no --format is specified the command acts as before.
There was a problem hiding this comment.
From what I gather the current behaviour is as below:
To decide where to print XML is not based on the --format but depends on the result of isXML(scanResults.Targets)
If we add --format xml it will mean if isXML(scanResults.Targets) == false we will have to convert non-XML data to XML and print it.
@attiasas should we do it ?
In this PR we only convert XML to json if --format==json and isXML(scanResults.Targets)==true.
cf39866 to
205a8cf
Compare
205a8cf to
6421052
Compare
Summary
Adds a
--formatflag to thesbom-enrich(aliasse) command, allowing the output format to be selected when the input SBOM is XML (CycloneDX). Currentlyjsonis the only supported value for the flag; when set, XML input is converted to JSON with the enriched vulnerability data appended.Changes
cli/scancommands.gosbom-enrichcommand withSupportedFormats: [Json]so the shared--formatflag is wired in.EnrichCommandvia the new setter.commands/enrich/enrich.gooutputFormatfield onEnrichCommandplusSetOutputFormat(...)setter.AppendVulnsFromXMLToJsonthat parses the XML SBOM, converts it to ordered JSON via the newxmlElementToOrderedJsonhelper, appends Xray vulnerabilities (bom-ref+ CVE id), and prints the result.Run()now branches on the requested format: XML input +--format=jsonproduces JSON output; otherwise the existing XML/JSON paths are preserved (backward compatible default).printVulnerabilitiesTablehelper for tabular vulnerability rendering.commands/enrich/enrich_test.gogo.mod/go.sumgithub.com/jfrog/jfrog-client-goto latest master (v1.55.1-0.20260505115216-b6c67f807bc3).Validation
devbranch.go vet ./....go fmt ./....commands/enrich/enrich_test.go.