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

fix: #953 Derive language from pURL - https://github.com/anchore/syft… #957

Merged
merged 7 commits into from
Apr 26, 2022
Merged

fix: #953 Derive language from pURL - https://github.com/anchore/syft… #957

merged 7 commits into from
Apr 26, 2022

Conversation

jonmcewen
Copy link
Contributor

…/issues/953

Signed-off-by: Jon McEwen jon_mcewen@hotmail.com

@jonmcewen
fix: anchore#953 Derive language from pURL - anchore#953
a24e35a 
Signed-off-by: Jon McEwen <jon_mcewen@hotmail.com>
@spiffcs
Copy link
Contributor

spiffcs commented Apr 22, 2022

Thanks for the contribution @jonmcewen. Approved and running the checks now

@spiffcs
Copy link
Contributor

spiffcs commented Apr 22, 2022

@jonmcewen I'm taking a look at the integration testing here to see what updates have to go in for this PR to be ok to merge

@jonmcewen
Copy link
Contributor Author

Thanks @spiffcs . Let me know if I need to do more. I had tests failing on master before the change so assumed that was something to do with my laptop setup

@spiffcs
Copy link
Contributor

spiffcs commented Apr 25, 2022

Just a quick update on this PR. The reason the tests are failing is because we test an encode/decode/encode cycle of the formats. The original encoding of the cyclonedx format will NOT include certain properties that get included after a decode.

Example:

Original encode will not do the PURL language check so properties like below

      	        {
         	          "name": "syft:package:language",
         	          "value": "UnknownLanguage"
         	    },

The reason they these properties are included on the second pass is that we decode the original. This decode does the new language check, which then encodes the new property which shows we could not find a language.

I'm going to ask our team today on if we want to include ("UnknownLanguage") properties in the original encode OR if we want to redact it and not include it when doing this language from CPE check.

@spiffcs
update cataloger path to encode more information
903b1ad 
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs spiffcs requested a review from a team April 25, 2022 15:47
@spiffcs
remove extra lines from test
9d26b47 
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
spiffcs
spiffcs approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@spiffcs spiffcs left a comment

Choose a reason for hiding this comment

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

Great addition to syft's ability to generate more complete information from outside SBOM.

Looking for a second from @anchore/tools

wagoodman
wagoodman reviewed Apr 25, 2022
View reviewed changes
syft/pkg/cataloger/catalog.go
@@ -69,6 +69,9 @@ func Catalog(resolver source.FileResolver, release *linux.Release, catalogers ..

// generate PURL (note: this is excluded from package ID, so is safe to mutate)
p.PURL = pkg.URL(p, release)
if p.Language == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: an extra comment explaining why this is here would be good since there is some high-level reasoning for it that isn't apparent.

@spiffcs
Copy link
Contributor

spiffcs commented Apr 25, 2022

Looks like my changes made the integration tests FAIL. I need to update the assertions so it sees it's ok now to haveUnknownLanguage

spiffcs added 2 commits April 26, 2022 10:31
@spiffcs
add unknown language as expected in distro test
780076e 
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs
add comment
1178bab 
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs
Copy link
Contributor

spiffcs commented Apr 26, 2022

@jonmcewen I'll make sure to get this over the line today. We really appreciate the contribution and the thought you put into this PR. I'm just trying to get some of the encode/decode patterns for different formats reconciled with this change.

spiffcs added 2 commits April 26, 2022 11:05
@spiffcs
redact id/parent from test output
36da8b0 
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs
update UnknownLanguage ==> ""
4d0a98f 
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs spiffcs merged commit 7304bbf into anchore:main Apr 26, 2022
@spiffcs spiffcs self-assigned this Apr 28, 2022
@jonmcewen
Copy link
Contributor Author

@spiffcs many thanks for getting this in and making it a pleasant contribution experience :-)

spiffcs added a commit that referenced this pull request May 2, 2022
@spiffcs
Merge branch 'main' into 835-keyless-attestation-upgrade
cb25858 
* main: (31 commits)
  reduce noise of log output (#976)
  add version info and remove double config call (#977)
  Rename syft-id to package-id (#970)
  update to cyclonedx-go 0.5.2 (#971)
  refactor command package to remove globals and add dependency injection
  fix: #953 Derive language from pURL - https://github.com/anchore/syft… (#957)
  Fix typo in CPE-parsing error (#966)
  Preserve syft IDs on SBOM decode (#963)
  Update GitHub format package_url and correlator (#961)
  Ensure SPDXIDs are valid (#955)
  Auto-PR needs to run go mod tidy (#958)
  Add workflow for automatic PR for new stereoscope updates (#954)
  Minor readme update to correct format information (#948)
  Update spdx22json to only take uppercase checksum algorithm (#946)
  add additional vendors for springframework (#945)
  Add digest property to parent and nested java package metadata (#941)
  Update write permissions and log into ghcr.io for release (#942)
  Retry auth URL lookup without docker credentialhelper workaround (#939)
  Ensure that all cyclonedx components have bom-refs (#914)
  Additionally publish docker images to GHCR (#934)
  ...

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
rigzba21 pushed a commit to rigzba21/syft that referenced this pull request May 5, 2022
@jonmcewen @rigzba21
fix: anchore#953 Derive language from pURL - https://github.com/ancho…
89b7c39 
…re/syft… (anchore#957)

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
@jonmcewen jonmcewen mentioned this pull request Aug 25, 2022
Closed
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
@jonmcewen
fix: anchore#953 Derive language from pURL - https://github.com/ancho…
2a7265a 
…re/syft… (anchore#957)

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wagoodman wagoodman wagoodman left review comments

@spiffcs spiffcs spiffcs approved these changes

Assignees

@spiffcs spiffcs

Labels
None yet
Projects
OSS
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonmcewen @spiffcs @wagoodman