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.

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

Update PackageVerificationCode.md #699

Merged
merged 1 commit into from
Apr 6, 2024
Merged

Update PackageVerificationCode.md #699

merged 1 commit into from
Apr 6, 2024

Conversation

kestewart
Copy link
Contributor

Add in Algorithm, and rework algorithm example to use example fields.

Signed-off-by: Kate Stewart kstewart@linuxfoundation.org

Add in Algorithm, and rework algorithm example to use example fields.
@kestewart kestewart requested review from goneall, zvr and maxhbr April 6, 2024 08:11
@kestewart
Copy link
Contributor Author

This addresses #646

@maxhbr
Copy link
Member

maxhbr commented Apr 6, 2024

I thought that the algorithm was fixed to SHA1 and is not parametric? Is there a reason to change this compared to v2? I thought that package verification code is also basically deprecated and discourage from being used. Making it more capable feels weird.

@goneall
Copy link
Member

goneall commented Apr 6, 2024

@maxhbr you raise a good point. I'm OK either way as this is just for legacy purposes.

It may be easier to migrate if we don't have the property and just define it as SHA1 since we don't have an algorithm property in 2.3.

@kestewart - thoughts?

@kestewart
Copy link
Contributor Author

It had been talked about using SHA256 as this will be more common than SHA1. At this point, prefer to leave it flexible with "algorithm", and can restrict it if necessary going forward.

@maxhbr
Copy link
Member

maxhbr commented Apr 6, 2024

we describe the PackageVerificationCode with

This verification method is provided for compatibility with SPDX 2.X.

Use of this verification code method is discouraged except for scenarios where the gitoid property on Artifact can not be used.

In my opinion, this makes complicated code that seldomly gets implemented correctly more complex.

Copy link
Member

@maxhbr maxhbr left a comment

Choose a reason for hiding this comment

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

The code looks fine, but I think this should not be merged.

Also the V2->V3 migration guide would need to be adopted for this change.

Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

LGTM - consistent with discussions on tech call

@goneall goneall merged commit 65b3c52 into main Apr 6, 2024
1 check passed
@goneall goneall deleted the kestewart-patch-1 branch April 6, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants