8000 feat: Export command - Export SBOM from storage by houdini91 · Pull Request #75 · bomctl/bomctl · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Export command - Export SBOM from storage #75

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Jun 26, 2024
Merged

feat: Export command - Export SBOM from storage #75

merged 15 commits into from
Jun 26, 2024

Conversation

houdini91
Copy link
Contributor
@houdini91 houdini91 commented Jun 11, 2024 •
edited
Loading

Export SBOMs Command

Description

Suggestion for a Save Command, Considering @jhoward-lm Comment.

Typically, users populate the database using the fetch command. This new command will allow users to save SBOMs to their local filesystem.

Usage suggestion.

Save SBOM file(s) from Storage to Filesystem

Usage:
  bomctl save [flags] SBOM_ID...

Flags:
  -e, --encoding string   the output encoding [spdx: [text, json] cyclonedx: [json] (default "json")
  -f, --format string     output format [spdx, spdx-2.3, cyclonedx, cyclonedx-1.0, cyclonedx-1.1, cyclonedx-1.2, cyclonedx-1.3, cyclonedx-1.4, cyclonedx-1.5]
  -h, --help              help for save

Issues

  1. Handling multiple SBOM IDs with a single output file:
    Option 1: Output as tar archive
    Option 2: Append index to file path
    Option 3: Do not support multiple SBOM IDs in the command
    Open to other suggestions

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Waiting for feedback before implementing tests.

@houdini91
temporary Save command - Save SBOM to File By ID
7f73dab 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91 houdini91 changed the title temporary Save command - Save SBOM to File By ID feat: Save command - Save SBOM to File By ID Jun 11, 2024
@idunbarh
Copy link
Member

@houdini91 Thanks for working on this PR! I think main is in a good spot to enable this command.

Let us know if you need anything.

CC/ @ashearin and @jhoward-lm

@jhoward-lm
Copy link
Contributor

After giving it some thought, I think it makes more sense to me for this sub command to be called export, where if no positional parameters are provided it should write to stdout for piping to other tools. I still think the fetch option should be called --save however. Thoughts?

The reason is for parity with a potential import command I'm kicking around, which would basically just be fetch except designed for local files and stdin input.

@idunbarh
Copy link
Member

I'm trying to mentally walk through how a user would use bomctl with multiple SBOMs or SBOMs with externalRefs to other SBOMs.

I think its worth creating an ADR outlining a common philosophy for "piping" multiple SBOMs from this command.

If the fetch command allows fetching of externalRefs, I think a save command that could put "tree structures" as sbom files.

If the import command allows a single SBOM to be piped in, I would expect an export to also pipeline a single SBOM.

@jhoward-lm
Copy link
Contributor
jhoward-lm commented Jun 13, 2024 •
edited
Loading

If the fetch command allows fetching of externalRefs, I think a save command that could put "tree structures" as sbom files.

What are your thoughts for the naming convention of the files in that tree structure, or ways to specify multiple output file names and map them to the input URLs

If the import command allows a single SBOM to be piped in, I would expect an export to also pipeline a single SBOM.

It might not necessarily have to be a single input or output SBOM, thinking along the lines of how tools like xargs handle these situations. Piped I nput or output could basically be a whitespace delimited series of multiple super long SBOM content strings

@houdini91
8000
Copy link
Contributor Author

In my opinion, large stdout and stdin strings are useful when using pipes but not very practical when printed directly to the terminal.

I also agree that the save command should be renamed to export. IMHO one if the use case is exporting an SBOM to a third-party system or generating a report. In such cases, outputting a collection/tree of SBOMs files may be useful and more natural to some systems.

I was under the impression that creating a unified SBOM should be the responsibility of the merge command, not the export/save command.

houdini91 added 6 commits June 24, 2024 13:36
@houdini91
save to export command
a4999b4 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91
tidy
307d6a0 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91
save to export command
8a2f85e 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91
save to export command
cde5308 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91
save to export command
e2c4437 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91
allow multiple sbomIDs
0d1c421 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91
Copy link
Contributor Author
houdini91 commented Jun 24, 2024 •
edited
Loading

Sorry for the delay, i was yet on another small vacation.

After giving it some thought, I think it makes more sense to me for this sub command to be called export, where if no positional parameters are provided it should write to stdout for piping to other tools. I still think the fetch option should be called --save however. Thoughts?

Changed save to export command as suggested.

It might not necessarily have to be a single input or output SBOM, thinking along the lines of how tools like xargs handle these situations. Piped I nput or output could basically be a whitespace delimited series of multiple super long SBOM content strings

If the import command allows a single SBOM to be piped in, I would expect an export to also pipeline a single SBOM.

I Patched the code with both these suggestions, what do you think?

  • When uses output-file option only one SBOM id is allowed for command.
  • When not using output-file option all Sboms are pushed out to stdout.

It seemed more in sync with the current fetch command flow.

Open Q:
Can someone elaborate on the tree structure in the fetch command, specifically what field is used as the Tree Node Unique identifier in a SBOM tree?

@jhoward-lm @idunbarh

@houdini91 houdini91 marked this pull request as ready for review June 24, 2024 11:20
@houdini91 houdini91 requested a review from a team as a code owner June 24, 2024 11:20
jhoward-lm
jhoward-lm requested changes Jun 24, 2024
View reviewed changes
Copy link
Contributor
@jhoward-lm jhoward-lm left a comment

Choose a reason for hiding this comment

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

Need to run golangci-lint locally and address findings

jhoward-lm
jhoward-lm requested changes Jun 24, 2024
View reviewed changes
houdini91 and others added 3 commits June 24, 2024 19:17
@houdini91 @jhoward-lm
Update internal/pkg/utils/format/format_test.go
cd4a6a6 
Co-authored-by: jhoward-lm <140011346+jhoward-lm@users.noreply.github.com>
Signed-off-by: mikey strauss <mdstrauss91@gmail.com>
@houdini91 @jhoward-lm
Update cmd/export.go
ae10243 
Co-authored-by: jhoward-lm <140011346+jhoward-lm@users.noreply.github.com>
Signed-off-by: mikey strauss <mdstrauss91@gmail.com>
@houdini91 @jhoward-lm
Update internal/pkg/utils/format/format.go
52e3d37 
Co-authored-by: jhoward-lm <140011346+jhoward-lm@users.noreply.github.com>
Signed-off-by: mikey strauss <mdstrauss91@gmail.com>
@idunbarh
Copy link
Member

@houdini91 we talked about this PR today at the community meeting. It looks like its in good shape.

There are two requests and then we're good to merge.

I realize this PR been open for a while, thank you for being patient with us!

houdini91 added 2 commits June 25, 2024 09:08
@houdini91
fix lint
588d33b 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91
normalize to list PR
5375d40 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91 houdini91 changed the title feat: Save command - Save SBOM to File By ID feat: Export command - Export SBOM from storage Jun 25, 2024
@houdini91
inital test export
db96d08 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91
Copy link
Contributor Author

@idunbarh @jhoward-lm I think i fixed all your comments.
Have another look at it.

I ran a local test and it worked as expected.

go run main.go fetch https://raw.githubusercontent.com/protobom/protobom/main/test/conformance/testdata/cyclonedx/1.5/json/bom-1.5.json --cache-dir .tmp

go run main.go export urn:uuid:3e671687-395b-41f5-a30f-a58921a69b79 --cache-dir .tmp

@houdini91
Copy link
Contributor Author

Current Status of usage.

Export SBOM file(s) from Storage

Usage:
  bomctl export [flags] SBOM_URL...

Flags:
  -e, --encoding STRING    the output encoding [spdx: [text, json] cyclonedx: [json] (default json)
  -f, --format STRING      Output Format:
                           	- spdx,
                           	- spdx-2.3,
                           	- cyclonedx,
                           	- cyclonedx-1.0,cyclonedx-1.1,
                           	- cyclonedx-1.2,
                           	- cyclonedx-1.3,
                           	- cyclonedx-1.4,
                           	- cyclonedx-1.5
                           	 (default cyclonedx-1.5)
  -h, --help               help for export
  -o, --output-file FILE   Path to output file

Global Flags:
      --cache-dir string   cache directory [defaults:
                           	Unix:    $HOME/.cache/bomctl
                           	Darwin:  $HOME/Library/Caches/bomctl
                           	Windows: %LocalAppData%\bomctl]
      --config string      config file [defaults:
                           	Unix:    $HOME/.config/bomctl/bomctl.yaml
                           	Darwin:  $HOME/Library/Application Support/bomctl/bomctl.yml
                           	Windows: %AppData%\bomctl\bomctl.yml]
  -v, --verbose            Enable debug output

@houdini91
Copy link
Contributor Author

@jhoward-lm @idunbarh

I decided to set the default format string to cyclonedx-1.5.

  • Should we consider changing the default to something else?
  • Do we retain the original document format somewhere that the command can use?

This might be related to protobom/protobom#213.

@idunbarh
Copy link
Member
idunbarh commented Jun 25, 2024 •
edited
Loading

@jhoward-lm @idunbarh

I decided to set the default format string to cyclonedx-1.5.

  • Should we consider changing the default to something else?
  • Do we retain the original document format somewhere that the command can use?

This might be related to protobom/protobom#213.

Having the default as cyclonedx-1.5 makes sense to me. I do agree that we need to resolve protobom/protobom#213 ideally within protobom or with this project, but lets solve that at a later time.

Also for the last linting issue I think changing the linting rules to allow the max cyclomatic complexity to 13 is reasonable (or take an exception and suppress error).

jhoward-lm
jhoward-lm requested changes Jun 25, 2024
View reviewed changes
Copy link
Contributor
@jhoward-lm jhoward-lm left a comment

Choose a reason for hiding this comment

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

Final suggestions to address lint findings and resolve conflicts with list PR

@jhoward-lm
Copy link
Contributor

Also for the last linting issue I think changing the linting rules to allow the max cyclomatic complexity to 13 is reasonable (or take an exception and suppress error).

I think the complexity rule is there for a reason to keep our code easy to read and understand, so I would prefer to avoid modifying the golangci-lint configs if at all possible.

The fix should be pretty simple, just create a private function for the first switch statement, something like

func getFormatMap(encoding string) (map[string]formats.Format, error) {
	switch encoding {
	case formats.JSON:
		return JSONFormatMap(), nil
	case formats.TEXT:
		return TVFormatMap(), nil
	case formats.XML:
		return XMLFormatMap(), nil
	default:
		return formats.EmptyFormat,
			errUnknownEncoding
	}
}

@houdini91
Copy link
Contributor Author

Lint passed Fhew...!

@jhoward-lm jhoward-lm requested review from eddiezane, ashearin, idunbarh, mfrystacky and a team June 25, 2024 17:18
@jhoward-lm jhoward-lm added the enhancement New feature or request label Jun 25, 2024
@idunbarh
Copy link
Member

We're so close! Looks like there is one commit that is not signed, which is blocking us from merging.

Can you squash and sign? We'll merge after that.

It will be great to get this command and round out the initial functionality. Thank you for all of the time spent on this :-D

jhoward-lm
jhoward-lm approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor
@jhoward-lm jhoward-lm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some last minute suggestions to leverage the ent client debugging if you get time.

EDIT: these can be done in a follow on issue if time is a factor

@houdini91
Export command squashed
7d2049d 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91 houdini91 force-pushed the suggestion/save-command branch from 11b2c17 to 7d2049d Compare June 26, 2024 14:00
@houdini91
Export command squashed
d1aa01b 
Signed-off-by: houdini91 <mdstrauss91@gmail.com>
@houdini91
Copy link
Contributor Author

@idunbarh @jhoward-lm Squashed offending commit, my bad.

@idunbarh idunbarh merged commit 295c923 into bomctl:main Jun 26, 2024
6 checks passed
idunbarh added a commit to idunbarh/bomctl that referenced this pull request Jun 26, 2024
@idunbarh
chore: minor updates for bomctl#75
9eb637e 
Signed-off-by: Ian Dunbar-Hall <ian.dunbar-hall@lmco.com>
@idunbarh idunbarh mentioned this pull request Jun 26, 2024
Merged
15 tasks
idunbarh added a commit that referenced this pull request Jun 26, 2024
@idunbarh
chore: minor updates for #75 (#85)
dea188f 
Signed-off-by: Ian Dunbar-Hall <ian.dunbar-hall@lmco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idunbarh idunbarh idunbarh approved these changes

@jhoward-lm jhoward-lm jhoward-lm approved these changes

@eddiezane eddiezane Awaiting requested review from eddiezane

@ashearin ashearin Awaiting requested review from ashearin

@mfrystacky mfrystacky Awaiting requested review from mfrystacky

Assignees

@houdini91 houdini91

Labels
enhancement New feature or request
Projects
bomctl
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@houdini91 @idunbarh @jhoward-lm
0