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

feat: add CPE to component #138

Merged
merged 8 commits into from Jan 24, 2022
Merged

feat: add CPE to component #138

merged 8 commits into from Jan 24, 2022

Conversation

jblu42
Copy link
Contributor

@jblu42 jblu42 commented Jan 20, 2022

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

fixes #139

@jblu42 jblu42 requested a review from a team as a code owner January 20, 2022 09:15
cyclonedx/model/component.py Outdated Show resolved Hide resolved
@jkowalleck
Copy link
Member

jkowalleck commented Jan 20, 2022

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?:

  1. JSON output is missing - XML output was done

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

  1. You need to sign your commits - please check CONTRIBUTING
  2. Few code stlye analysis violations have been reported
  3. 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
Copy link
Contributor Author

jblu42 commented Jan 20, 2022

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.

@jkowalleck
Copy link
Member

jkowalleck commented Jan 20, 2022

#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 jkowalleck added the enhancement New feature or request label Jan 20, 2022
@jkowalleck jkowalleck changed the title Added CPE to component feat: add CPE to component Jan 20, 2022
@jkowalleck
Copy link
Member

@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
Copy link
Contributor Author

jblu42 commented Jan 21, 2022

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
Copy link
Member

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 <jens.lucius@de.bosch.com>
- Fixed styling errors
- Added reference to CPE Spec
- Adding CPE parameter as last parameter to not break arguments

Signed-off-by: Jens Lucius <jens.lucius@de.bosch.com>
Missing in the last commit

Signed-off-by: Jens Lucius <jens.lucius@de.bosch.com>
Signed-off-by: Jens Lucius <jens.lucius@de.bosch.com>
- 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 <jens.lucius@de.bosch.com>
CPE was still in the wrong position in one of the tests - fixed

Signed-off-by: Jens Lucius <jens.lucius@de.bosch.com>
Copy link
Collaborator

@madpah madpah left a comment

Choose a reason for hiding this comment

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

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

FYI @jkowalleck

tests/fixtures/bom_v1.2_setuptools_with_cpe.json Outdated Show resolved Hide resolved
tests/fixtures/bom_v1.4_setuptools_with_cpe.json Outdated Show resolved Hide resolved
- cpe was still in wrong position in 1.2 JSON
- Indentation fixed in 1.4 JSON

Signed-off-by: Jens Lucius <jens.lucius@de.bosch.com>
@madpah
Copy link
Collaborator

madpah commented Jan 24, 2022

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 <jens.lucius@de.bosch.com>
@jkowalleck
Copy link
Member

PR looks solid. approval from my side.

@madpah madpah mentioned this pull request Jan 24, 2022
@madpah madpah merged commit 269ee15 into CycloneDX:main Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add possibility to use CPE in components
3 participants