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 cyclonedx.model.dependency.Dependency.provides #735

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

uzairchhapra
Copy link

Fixes #691

@uzairchhapra uzairchhapra force-pushed the feat/691-uzair-provides branch from f2ad0ed to fb4598d Compare November 5, 2024 19:11
@uzairchhapra uzairchhapra marked this pull request as ready for review November 5, 2024 21:47
@uzairchhapra uzairchhapra requested a review from a team as a code owner November 5, 2024 21:47
@uzairchhapra
Copy link
Author

@jkowalleck PR is ready for review.
I am not too sure about the test cases so any guidance here would help. Thanks!

@jkowalleck jkowalleck changed the title [WIP] feat: add cyclonedx.model.dependency.Dependency.provides feat: add cyclonedx.model.dependency.Dependency.provides Nov 5, 2024
@jkowalleck
Copy link
Member

thank you for your contribution, @uzairchhapra .

the implementation looks promising.

Regarding tests, we tend to go with an integration-test snapshot-solution, over detailed unit tests.
Please add new fixtures to https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/tests/_data/models.py. your new function must start with get_bom_.
After adding new test data, please recreate the snapshots as described here: https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/tests/_data/snapshots/README.md

@jkowalleck

This comment was marked as outdated.

self,
target: Dependable,
depends_on: Optional[Iterable[Dependable]] = None,
provides: Optional[Iterable[Dependable]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

instead of adding a new parameter here, how about adding a new method instead: register_provision(self, target: Dependable, provides: Optional[Iterable[Dependable]] = None).

what do you think about this?
this would fit the original architectural plans better.

Copy link
Author

Choose a reason for hiding this comment

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

I'll give this a try

tests/_data/models.py Show resolved Hide resolved
@@ -0,0 +1,113 @@
{
Copy link
Author

Choose a reason for hiding this comment

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

How do I restrict the tests to only pickup the provides parameter in Dependency when schema is 1.6? When I re-ran the snapshots I can see 10 errors:

  1. 5 for schema v1.0 to v1.5 JSON
  2. 5 for schema v1.0 to v1.5 XML
    image

Copy link
Member

@jkowalleck jkowalleck Nov 12, 2024

Choose a reason for hiding this comment

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

this could be fixed by a view filter.
here is an example:

@property
@serializable.view(SchemaVersion1Dot2)
@serializable.view(SchemaVersion1Dot3)
@serializable.view(SchemaVersion1Dot4)
@serializable.view(SchemaVersion1Dot5)
@serializable.view(SchemaVersion1Dot6)
@serializable.xml_sequence(6)
def manufacture(self) -> Optional[OrganizationalEntity]:

Copy link
Author

Choose a reason for hiding this comment

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

I tried adding it here:

@serializable.view(SchemaVersion1Dot6)

The tests still seem to fail for all other SBOM specs (v1.0 to v1.5).

I am sure I must be missing something here...

Copy link
Member

Choose a reason for hiding this comment

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

@jkowalleck jkowalleck self-requested a review November 12, 2024 12:54
@@ -1310,6 +1310,28 @@ def get_bom_with_definitions_standards() -> Bom:
)


def get_bom_v1_6_with_provides() -> Bom:
Copy link
Member

@jkowalleck jkowalleck Nov 18, 2024

Choose a reason for hiding this comment

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

please rename to get_bom_with_provides.


there is no intention to have models for certain CDX versions only.
In fact, it is intended to test the serialization with a target that is expected to omit certain parts.

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.

feat: add cyclonedx.model.dependency.Dependency.provides
2 participants