feat: add CPE to component #138

jblu42 · 2022-01-20T09:15:07Z

Setting CPE was missing for component, now it is possible to set CPE and output CPE for a component.

fixes #139

cyclonedx/model/component.py

jkowalleck · 2022-01-20T09:41:37Z

Thank you for the PullRequest, @jblu42.
This looks like a very good idea.

Can you open an Issue Ticket to describe the feature/fix etc. that you are contributing please? It will help us (and the community) understand and track the change.

Could you fix the following functional points?:

JSON output is missing - XML output was done

Aside from the PR comments, there are a couple of other hygiene points to look at:

You need to sign your commits - please check CONTRIBUTING
Few code stlye analysis violations have been reported
add tests for the following scenario
- CPE is set to None - test setter/getter/constructor
- CPE is set to some non-None value - test setter/getter/constructor
- test JSON output with a non-None value
- test XML output with a non-None value

Thanks again and don't hesitate to ask any questions.

do you have any additional remarks, @madpah ?

jblu42 · 2022-01-20T10:06:30Z

Sorry forgot some fixes in the last commit, now added.

I think for JSON output there are no changes required as it uses the JSON schema as reference and the CPE is included in there. At least in my tests the JSON output is working.

Are the tests required? There are no tests for the other optional attributes, but if you like I can add some, just need to look how the tests are defined.

cyclonedx/model/component.py

jkowalleck · 2022-01-20T10:30:17Z

#138 (comment)

Sorry forgot some fixes in the last commit, now added.

I think for JSON output there are no changes required as it uses the JSON schema as reference and the CPE is included in there. At least in my tests the JSON output is working.

Are the tests required? There are no tests for the other optional attributes, but if you like I can add some, just need to look how the tests are defined.

If you can, please add a test for the JSON and for the XML output, that includes a non-None CPE value. that should be good enough for now.
If you think additional tests would support the feature you provide, feel free to add them, too.

jkowalleck · 2022-01-20T18:20:46Z

@jblu42 ,
Could you get a couple of formals sorted out?

some commits are not signed. See the "DCO" test results for details.
some test are failing. see the test results for details.

jblu42 · 2022-01-21T06:48:39Z

Strange, I did run it through poetry tests before committing, will have a look at it again. The problem with the sign off is my first commit, who was not signed off, do you know how to correct that, I am not using git so often?

jkowalleck · 2022-01-21T08:05:51Z

Strange, I did run it through poetry tests before committing, will have a look at it again. The problem with the sign off is my first commit, who was not signed off, do you know how to correct that, I am not using git so often?

Hello @jblu42 ,
By following the steps DCO checker described in the "details" link in the test summary you should be all good.
See for example: https://github.com/CycloneDX/cyclonedx-python-lib/pull/138/checks?check_run_id=4893513908

Setting CPE was missing for component, now it is possible to set CPE and output CPE for a component. Signed-off-by: Jens Lucius <[email protected]>

- Fixed styling errors - Added reference to CPE Spec - Adding CPE parameter as last parameter to not break arguments Signed-off-by: Jens Lucius <[email protected]>

Missing in the last commit Signed-off-by: Jens Lucius <[email protected]>

Signed-off-by: Jens Lucius <[email protected]>

- Added output tests for CPE in XML and JSON - Fixes style error in components - Fixes order for CPE output in XML (CPE has to come before PURL) Signed-off-by: Jens Lucius <[email protected]>

CPE was still in the wrong position in one of the tests - fixed Signed-off-by: Jens Lucius <[email protected]>

madpah

This is looking great. Couple of very minor things in tests.

FYI @jkowalleck

tests/fixtures/bom_v1.2_setuptools_with_cpe.json

tests/fixtures/bom_v1.3_setuptools_with_cpe.json

tests/fixtures/bom_v1.4_setuptools_with_cpe.json

- cpe was still in wrong position in 1.2 JSON - Indentation fixed in 1.4 JSON Signed-off-by: Jens Lucius <[email protected]>

madpah · 2022-01-24T09:33:59Z

Thanks again @jblu42 - we'll just wait for @jkowalleck to come back regarding positioning of the new cpe parameter - but this is looking great and we really really appreciate the contribution.

Signed-off-by: Jens Lucius <[email protected]>

jkowalleck · 2022-01-24T12:10:09Z

PR looks solid. approval from my side.

jblu42 requested a review from a team as a code owner January 20, 2022 09:15

jkowalleck requested changes Jan 20, 2022

View reviewed changes

cyclonedx/model/component.py Outdated Show resolved Hide resolved

jkowalleck reviewed Jan 20, 2022

View reviewed changes

cyclonedx/model/component.py Outdated Show resolved Hide resolved

jkowalleck reviewed Jan 20, 2022

View reviewed changes

cyclonedx/model/component.py Show resolved Hide resolved

jkowalleck requested a review from madpah January 20, 2022 09:41

jkowalleck reviewed Jan 20, 2022

View reviewed changes

cyclonedx/model/component.py Outdated Show resolved Hide resolved

jkowalleck reviewed Jan 20, 2022

View reviewed changes

cyclonedx/model/component.py Outdated Show resolved Hide resolved

jkowalleck added the enhancement New feature or request label Jan 20, 2022

jkowalleck changed the title ~~Added CPE to component~~ feat: add CPE to component Jan 20, 2022

jblu42 force-pushed the main branch from 9c2965e to e3c392a Compare January 21, 2022 08:04

jblu42 force-pushed the main branch from e3c392a to 88b3411 Compare January 21, 2022 08:06

jblu42 added 6 commits January 21, 2022 09:32

Added CPE to component

80622b2

Setting CPE was missing for component, now it is possible to set CPE and output CPE for a component. Signed-off-by: Jens Lucius <[email protected]>

Fixing problems with CPE addition

55c9eae

- Fixed styling errors - Added reference to CPE Spec - Adding CPE parameter as last parameter to not break arguments Signed-off-by: Jens Lucius <[email protected]>

Again fixes for Style and CPE reference

eb405c3

Missing in the last commit Signed-off-by: Jens Lucius <[email protected]>

Added CPE as argument before deprecated arguments

80209cc

Signed-off-by: Jens Lucius <[email protected]>

Added testing for CPE addition and error fixing

134b1e3

- Added output tests for CPE in XML and JSON - Fixes style error in components - Fixes order for CPE output in XML (CPE has to come before PURL) Signed-off-by: Jens Lucius <[email protected]>

Fixed output tests

18f39af

CPE was still in the wrong position in one of the tests - fixed Signed-off-by: Jens Lucius <[email protected]>

jblu42 force-pushed the main branch from 88b3411 to 18f39af Compare January 21, 2022 08:33

jkowalleck approved these changes Jan 22, 2022

View reviewed changes

madpah requested changes Jan 24, 2022

View reviewed changes

Fixed minor test fixtures issues

5652e2e

- cpe was still in wrong position in 1.2 JSON - Indentation fixed in 1.4 JSON Signed-off-by: Jens Lucius <[email protected]>

Fixed missing comma in JSON 1.2 test file

7c78f62

Signed-off-by: Jens Lucius <[email protected]>

madpah mentioned this pull request Jan 24, 2022

[BREAKING] Adopt PEP-3102 #143

Closed

madpah approved these changes Jan 24, 2022

View reviewed changes

madpah merged commit 269ee15 into CycloneDX:main Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add CPE to component #138

feat: add CPE to component #138

jblu42 commented Jan 20, 2022 •

edited by jkowalleck

Loading

jkowalleck commented Jan 20, 2022 •

edited

Loading

jblu42 commented Jan 20, 2022

jkowalleck commented Jan 20, 2022 •

edited

Loading

jkowalleck commented Jan 20, 2022

jblu42 commented Jan 21, 2022

jkowalleck commented Jan 21, 2022

madpah left a comment

madpah commented Jan 24, 2022

jkowalleck commented Jan 24, 2022

feat: add CPE to component #138

feat: add CPE to component #138

Conversation

jblu42 commented Jan 20, 2022 • edited by jkowalleck Loading

jkowalleck commented Jan 20, 2022 • edited Loading

jblu42 commented Jan 20, 2022

jkowalleck commented Jan 20, 2022 • edited Loading

jkowalleck commented Jan 20, 2022

jblu42 commented Jan 21, 2022

jkowalleck commented Jan 21, 2022

madpah left a comment

Choose a reason for hiding this comment

madpah commented Jan 24, 2022

jkowalleck commented Jan 24, 2022

jblu42 commented Jan 20, 2022 •

edited by jkowalleck

Loading

jkowalleck commented Jan 20, 2022 •

edited

Loading

jkowalleck commented Jan 20, 2022 •

edited

Loading