Skip to content

feat: add cyclonedx.model.dependency.Dependency.provides #735

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

feat: add cyclonedx.model.dependency.Dependency.provides #735

wants to merge 10 commits into from

Conversation

uzairchhapra
Copy link

Fixes #691

@uzairchhapra
feat: add cyclonedx.model.dependency.Dependency.provides
fb4598d 
Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
@uzairchhapra uzairchhapra force-pushed the feat/691-uzair-provides branch from f2ad0ed to fb4598d Compare November 5, 2024 19:11
@uzairchhapra
chore(test): add unit test test_dependency_with_provides
00536a7 
Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
@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 jkowalleck mentioned this pull request Nov 10, 2024
Open
3 tasks
@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

@uzairchhapra
ft: update validate and register_dependency to read provides
abf6abf 
Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
@uzairchhapra
chore: set view schema to 1.6 for provides
8cdb39b 
Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
@uzairchhapra
chore(test): add test get_bom_with_provides_valid
2525660 
Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
@jkowalleck

This comment was marked as outdated.

Sign in to view
jkowalleck
jkowalleck reviewed Nov 11, 2024
View reviewed changes
cyclonedx/model/bom.py
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

Copy link
Author

Choose a reason for hiding this comment

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

@jkowalleck
How should this function work register_provision for the example mentioned pin models.py?

A -- requires -> C
B -- provides -> C

Considering register_dependency and register_provision will be called for all A, B and C, should I look for existing dependencies added in register_provision to avoid creating a new Dependency?

Copy link
Member

Choose a reason for hiding this comment

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

this is still open.
could you consider adding a new method?

@jkowalleck jkowalleck self-requested a review November 12, 2024 12:54
@uzairchhapra
fix: rename test to apply only for v1.6
bd370af 
Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
jkowalleck
jkowalleck reviewed Nov 18, 2024
View reviewed changes
@jkowalleck
Copy link
Member

@uzairchhapra, I just wanted to ask how things are going.
Do you need any help with one of the open discussions/points?

@uzairchhapra
Copy link
Author

@uzairchhapra, I just wanted to ask how things are going. Do you need any help with one of the open discussions/points?

Apologies for the delayed response. I will get to this this weekend. If I remember correctly, I was stuck in making the test function work get_bom_v1_6_with_provides. I see my PR has some conflicts now, so will try to address that too.

@jkowalleck jkowalleck mentioned this pull request Jan 29, 2025
Open
60 tasks
@aespogom
Copy link

Hi everyone,
Thanks a lot for your amazing work!!
Just wanted to check in—I'm currently waiting on this fix to be released, as it's blocking some of our work. Really appreciate all the efforts from the maintainers and contributors here. Is there any estimated timeline for when this might be included in a release? Thanks in advance!

@robertlagrant
Copy link

@aespogom I'm not affiliated, but it looks as though the author has abandoned this PR. If it's blocking you, would you consider picking it up?

@uzairchhapra
Merge remote-tracking branch 'upstream/main' into feat/691-uzair-prov…
5bdc59d 
…ides

Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
@uzairchhapra
chore(test): add test get_bom_with_provides
7b64985 
Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
@uzairchhapra
chore(test): add snapshots for get_bom_with_provides
3b4f790 
Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
@uzairchhapra
chore(test): updated model get_bom_with_provides
39a5f8f 
Signed-off-by: Uzair Chhapra <uzairchhapra@gmail.com>
jkowalleck
jkowalleck requested changes Apr 11, 2025
View reviewed changes
cyclonedx/model/bom.py
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.

this is still open.
could you consider adding a new method?

@jkowalleck jkowalleck marked this pull request as draft April 23, 2025 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkowalleck jkowalleck jkowalleck requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: add cyclonedx.model.dependency.Dependency.provides
4 participants
@uzairchhapra @jkowalleck @aespogom @robertlagrant