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.

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

Attach hasSBOM nodes to artifacts instead of packages #1883

Merged
merged 8 commits into from
May 14, 2024
Merged

Attach hasSBOM nodes to artifacts instead of packages #1883

merged 8 commits into from
May 14, 2024

Conversation

nchelluri
Copy link
Contributor

@nchelluri nchelluri commented Apr 29, 2024 •
edited
Loading

Description of the PR

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If OpenAPI spec is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

@nchelluri nchelluri force-pushed the nchelluri/attach-hassbom-to-artifact branch from 21544b4 to e7823f0 Compare April 30, 2024 14:50
@nchelluri nchelluri changed the title Attach hasSBOM nodes to artifacts instead of packages (WIP) Attach hasSBOM nodes to artifacts instead of packages Apr 30, 2024
@nchelluri nchelluri marked this pull request as ready for review April 30, 2024 14:54
@nchelluri nchelluri force-pushed the nchelluri/attach-hassbom-to-artifact branch from e7823f0 to 2ffed5a Compare April 30, 2024 15:23
pxp928
pxp928 previously approved these changes May 1, 2024
View reviewed changes
Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

LGTM

@nchelluri nchelluri force-pushed the nchelluri/attach-hassbom-to-artifact branch from 738c086 to b9d9f9b Compare May 1, 2024 16:39
@nchelluri nchelluri requested a review from pxp928 May 1, 2024 16:58
@pxp928 pxp928 dismissed their stale review May 1, 2024 17:13

changes

@nchelluri nchelluri marked this pull request as draft May 1, 2024 17:52
@pull-request-size pull-request-size bot added size/XL and removed size/L labels May 1, 2024
@nchelluri
Copy link
Contributor Author

nchelluri commented May 1, 2024 •
edited
Loading

Edit: after some discussion with Parth I reworked the PR and force pushed, replacing the last two commits with 6fbda04. Things should look better now and the comment that used to be here can now be ignored.

@nchelluri nchelluri force-pushed the nchelluri/attach-hassbom-to-artifact branch from b1dcb2e to 6fbda04 Compare May 1, 2024 18:58
@nchelluri nchelluri marked this pull request as ready for review May 1, 2024 19:18
pxp928
pxp928 approved these changes May 1, 2024
View reviewed changes
Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

LGTM

mdeicas
mdeicas reviewed May 2, 2024
View reviewed changes
Copy link
Collaborator

@mdeicas mdeicas 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 picking this up @nchelluri! A few comments

pkg/ingestor/parser/spdx/parse_spdx.go Outdated Show resolved Hide resolved
pkg/ingestor/parser/spdx/parse_spdx.go Outdated Show resolved Hide resolved
pkg/ingestor/parser/spdx/parse_spdx.go Outdated Show resolved Hide resolved
pkg/ingestor/parser/spdx/parse_spdx.go Outdated Show resolved Hide resolved
@nchelluri
Copy link
Contributor Author

Thanks for the review Marco; I think your suggestions will improve the PR, and I aim to have them implemented over the next little while. I will re-request your review at that point.

@nchelluri nchelluri force-pushed the nchelluri/attach-hassbom-to-artifact branch from 6fbda04 to e880067 Compare May 6, 2024 14:08
@pull-request-size pull-request-size bot added size/L and removed size/XL labels May 6, 2024
@nchelluri
Attach hasSBOM nodes to artifacts instead of packages
c7b79cc 
- If possible (i.e. a digest is available for the subject of
  an SBOM), hasSBOM nodes will be attached to artifacts now,
  not packages.
- Also removed some unneeded parser map accessor funcs, and
  added a slice utility func used in this branch.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Fix top level artifacts not being added with the DOCUMENT key
8009a8c 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Update tests to cover new HasSBOM artifact behavior
5cedcf4 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Fix SPDX file artifact parsing
678af41 
- In this PR I introduced a bug where files in the SBOM were not
  promoted to top-level Document file artifacts and packages even
  if there was a relationship that indicated they were such. I
  fixed that here.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Call s.getTopLevelSPDXIDs() just once and store it
f73e09b 
- We pass it into both callers instead of calling it in
  each one.
- Also rename the function since:
  1. It is not just getting package SPIDs anymore.
  2. We want to comply with https://go.dev/wiki/CodeReviewComments#initialisms

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Simplify collection of top-level components
3bb23ee 
- We don't actually need maps for this as we only ever
  access the key SPDXRef-DOCUMENT within those maps.
- So make them slices. And we need just one slice for
  top-level artifacts, be they from packages or files.
- This makes it possible to delete the slice concat
  utilty func as well.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Make test a bit clearer
2ae8795 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri nchelluri force-pushed the nchelluri/attach-hassbom-to-artifact branch from e880067 to 2ae8795 Compare May 10, 2024 13:16
@nchelluri
Copy link
Contributor Author

@mdeicas can you take a look again please?

@pxp928 pxp928 requested a review from mdeicas May 13, 2024 19:15
@pxp928 pxp928 added the needs-review Needs writer LGTM label May 13, 2024
@nchelluri nchelluri force-pushed the nchelluri/attach-hassbom-to-artifact branch 2 times, most recently from c37ccff to bb49159 Compare May 14, 2024 01:44
@nchelluri
Log if t-l art count differs from t-l pkg count
a2bfc76 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri nchelluri force-pushed the nchelluri/attach-hassbom-to-artifact branch from bb49159 to a2bfc76 Compare May 14, 2024 01:46
@kodiakhq kodiakhq bot merged commit efa328a into guacsec:main May 14, 2024
8 checks passed
@nchelluri nchelluri deleted the nchelluri/attach-hassbom-to-artifact branch May 15, 2024 19:33
arorasoham9 pushed a commit to arorasoham9/guac that referenced this pull request May 17, 2024
@nchelluri @arorasoham9
Attach hasSBOM nodes to artifacts instead of packages (guacsec#1883)
17d82b4 
* Attach hasSBOM nodes to artifacts instead of packages

- If possible (i.e. a digest is available for the subject of
  an SBOM), hasSBOM nodes will be attached to artifacts now,
  not packages.
- Also removed some unneeded parser map accessor funcs, and
  added a slice utility func used in this branch.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Fix top level artifacts not being added with the DOCUMENT key

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Update tests to cover new HasSBOM artifact behavior

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Fix SPDX file artifact parsing

- In this PR I introduced a bug where files in the SBOM were not
  promoted to top-level Document file artifacts and packages even
  if there was a relationship that indicated they were such. I
  fixed that here.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Call s.getTopLevelSPDXIDs() just once and store it

- We pass it into both callers instead of calling it in
  each one.
- Also rename the function since:
  1. It is not just getting package SPIDs anymore.
  2. We want to comply with https://go.dev/wiki/CodeReviewComments#initialisms

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Simplify collection of top-level components

- We don't actually need maps for this as we only ever
  access the key SPDXRef-DOCUMENT within those maps.
- So make them slices. And we need just one slice for
  top-level artifacts, be they from packages or files.
- This makes it possible to delete the slice concat
  utilty func as well.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Make test a bit clearer

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Log if t-l art count differs from t-l pkg count

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

---------

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
Signed-off-by: Soham Arora <arorasoham9@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdeicas mdeicas mdeicas approved these changes

@pxp928 pxp928 pxp928 approved these changes

@lumjjb lumjjb Awaiting requested review from lumjjb

Assignees
No one assigned
Labels
needs-review Needs writer LGTM size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nchelluri @mdeicas @pxp928