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

fix: add missing Vulnerability.properties types in schema 1.4 #148

Merged
merged 3 commits into from Jul 20, 2022

Conversation

desenna
Copy link
Contributor

@desenna desenna commented May 21, 2022

Closes #147

@desenna desenna marked this pull request as ready for review May 25, 2022 00:53
Signed-off-by: Mike de Senna <desenna@gmail.com>
Signed-off-by: Mike de Senna <desenna@gmail.com>
Signed-off-by: Mike de Senna <desenna@gmail.com>
Copy link
Member

@jkowalleck jkowalleck left a comment

Choose a reason for hiding this comment

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

see my annotations.
please have them sorted out

@desenna
Copy link
Contributor Author

desenna commented May 28, 2022

see my annotations. please have them sorted out

@jkowalleck Thanks for the review. I responded to the comments with explanations, but don't understand what's wrong with them. Please feel free to elaborate on what specifically I'm getting wrong and I'll be happy to fix them. Also, if you know how to develop / test locally, can you please share that info with me so I can do? It's possible / likely that I made a mistake here somewhere, but I didn't see how to run tests in the instructions and afaik CI hasn't kicked off fully here so I can't see what steps are taken to setup and run tests. Thanks again!

@desenna desenna requested a review from jkowalleck May 28, 2022 13:25
@jkowalleck
Copy link
Member

@stevespringett can kick off the CI.

@desenna
Copy link
Contributor Author

desenna commented Jun 2, 2022

@jkowalleck @stevespringett I'm not able to understand what is causing the test to fail. I can run it locally but I don't see anything except that false is not true. Do you have any advice on how to proceed with this?

@stevespringett
Copy link
Member

@desenna don't worry about the failure. The CycloneDX Core Java library is the thing that tests it, and it uses local schemas, not the updates you've submitted.

@desenna
Copy link
Contributor Author

desenna commented Jun 3, 2022

@desenna don't worry about the failure. The CycloneDX Core Java library is the thing that tests it, and it uses local schemas, not the updates you've submitted.

Thanks @stevespringett makes sense.

So are there any other steps I should take here? Or is it ready to merge and deploy you think? Asking only because I can then get to work on resolving CycloneDX/cyclonedx-go#40

Thanks and have a great weekend!

@stevespringett
Copy link
Member

stevespringett commented Jun 3, 2022

@desenna Although this is a defect, it will have a negative impact on everyone using v1.4 at the moment. In order for this to move forward, the core working group would need to vote on incorporating this into v1.4.2 of the XML schema, or wait until v1.5 next year. The next Core Working Group meeting is on Monday June 6. I'll bring this topic up then.

@desenna
Copy link
Contributor Author

desenna commented Jun 3, 2022

@desenna Although this is a defect, it will have a negative impact on everyone using v1.4 at the moment. In order for this to move forward, the core working group would need to vote on incorporating this into v1.4.2 of the XML schema, or wait until v1.5 next year. The next Core Working Group meeting is on Monday June 6. I'll bring this topic up then.

SGTM! I agree this is a good idea to bring up during Core Working Group meeting and that a v1.4.2 patch version makes sense here.

Another option might even be to leave it as v1.4.1 (if that's where it's currently at) since this doesn't represent a breaking or backwards-incompatible change but instead completes the specification for XML and protobuf, whereas incrementing a patch version to v1.4.2 in my mind is more like a fix of something that was incorrect. The semantics are seemingly minor, but seems nothing was wrong here.. just incomplete by one field.

This carries the side benefit of also giving other library maintainers - like cyclonedx-go and others - the room/time to make changes in those libraries as well, which their consumers would be able to leverage or even make the open source contributions themselves!

@stevespringett
Copy link
Member

Ok this will be merged in later this week, but not released yet. We will be updating all official libraries with the updated schema and releasing all updates around the same time.

@desenna
Copy link
Contributor Author

desenna commented Jul 16, 2022

Ok this will be merged in later this week, but not released yet. We will be updating all official libraries with the updated schema and releasing all updates around the same time.

HI @stevespringett just to confirm is this the fix for java? CycloneDX/cyclonedx-core-java#208

@stevespringett
Copy link
Member

Confirmed. Had vacation, then COVID, but back at it now. Will merge in the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

properties element missing for vulnerability type in XML schema
3 participants