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: properly parse conan ref and include user and channel #2034

Merged
merged 2 commits into from
Aug 23, 2023
Merged

fix: properly parse conan ref and include user and channel #2034

merged 2 commits into from
Aug 23, 2023

Conversation

Pro
Copy link
Contributor

@Pro Pro commented Aug 17, 2023

The current implementation in syft is not parsing the user and channel from a conan ref.

Both are highly relevant for the resulting package version.

https://github.com/CycloneDX/cyclonedx-conan is parsing the user and channel and putting it in the resulting file.

With this MR the same functionality is also added to syft.

@Pro Pro force-pushed the fix-conan-ref-parse branch from 163e3d7 to 9f32dff Compare August 17, 2023 12:59
kzantow
kzantow reviewed Aug 17, 2023
View reviewed changes
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

This looks like a great start! Would you be able to add a test with some realistic samples of conanfile and lockfile entries?

syft/pkg/cataloger/cpp/package.go Outdated Show resolved Hide resolved
syft/pkg/cataloger/cpp/package.go Outdated Show resolved Hide resolved
syft/pkg/cataloger/cpp/package.go Outdated Show resolved Hide resolved
syft/pkg/cataloger/cpp/package.go Outdated Show resolved Hide resolved
syft/pkg/cataloger/cpp/package.go Outdated Show resolved Hide resolved
@Pro Pro force-pushed the fix-conan-ref-parse branch from 9f32dff to 5a4f877 Compare August 17, 2023 14:10
@Pro
Copy link
Contributor Author

Pro commented Aug 17, 2023

This looks like a great start! Would you be able to add a test with some realistic samples of conanfile and lockfile entries?

Done, added some tests @kzantow

@Pro Pro force-pushed the fix-conan-ref-parse branch from 5a4f877 to 9d21265 Compare August 17, 2023 14:11
kzantow
kzantow reviewed Aug 17, 2023
View reviewed changes
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

Thanks @Pro, I think the changes to the tests look sufficient; just one minor issue that wasn't resolved

syft/pkg/cataloger/cpp/package.go Outdated Show resolved Hide resolved
@kzantow
Copy link
Contributor

kzantow commented Aug 17, 2023

@Pro -- you should be able to fix the static analysis failure by running make lint-fix

@Pro Pro force-pushed the fix-conan-ref-parse branch from 9d21265 to 97072c5 Compare August 17, 2023 17:33
@kzantow
Copy link
Contributor

kzantow commented Aug 17, 2023

@Pro it looks like the last commit was not signed-off; could you rebase against main and force push these changes with sign-off?

@Pro
fix: properly parse conan ref and include user and channel
0b7ad64 
Signed-off-by: Stefan Profanter <stefan.profanter@agile-robots.com>
@Pro Pro force-pushed the fix-conan-ref-parse branch from 97072c5 to 0b7ad64 Compare August 17, 2023 18:25
@Pro Pro requested a review from kzantow August 22, 2023 13:47
@kzantow
Copy link
Contributor

kzantow commented Aug 22, 2023

Thanks for this PR @Pro -- there is one very small request to not export the ConanRef type. That's the last hold up for this PR, I think.

Do you happen to know much about Conan lockfiles version 0.5? I've been trying to find information about the schema, but I've been unsuccessful. There is a report that the format may have changed significantly in an incompatible manner, and we'd like to make sure to support both older and newer versions.

@wagoodman
unexport the conanRef type
b034707 
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
wagoodman
wagoodman approved these changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

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

I made the small tweak that @kzantow was referring to, otherwise it looks great! Thanks for the improvement here!

@wagoodman wagoodman enabled auto-merge (squash) August 23, 2023 17:46
@wagoodman wagoodman merged commit 07ac640 into anchore:main Aug 23, 2023
@kzantow kzantow added the bug Something isn't working label Aug 25, 2023
This was referenced Aug 28, 2023
Closed
Closed
Closed
Closed
Merged
Closed
Closed
@dependabot dependabot bot mentioned this pull request Sep 4, 2023
Closed
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
@Pro @wagoodman
fix: properly parse conan ref and include user and channel (anchore#2034
abcf04a 
)

* fix: properly parse conan ref and include user and channel

Signed-off-by: Stefan Profanter <stefan.profanter@agile-robots.com>

* unexport the conanRef type

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

---------

Signed-off-by: Stefan Profanter <stefan.profanter@agile-robots.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Co-authored-by: Alex Goodman <wagoodman@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kzantow kzantow kzantow left review comments

@wagoodman wagoodman wagoodman approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Pro @kzantow @wagoodman