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 support for Lifecycles in BOM metadata #698

Merged
merged 14 commits into from
Oct 21, 2024

Conversation

Churro
Copy link
Contributor

@Churro Churro commented Oct 9, 2024

part of #578

@Churro Churro requested a review from a team as a code owner October 9, 2024 22:40
Signed-off-by: Johannes Feichtner <johannes@web-wack.at>
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.

The implementation is really well thought and has almost every small detail perfectly correct. 🥇

There are just a very small amount of things that needs changing, marked with ❌ .

In addition, please add the needed enum tests to tests/test_enums.py.

cyclonedx/model/bom.py Show resolved Hide resolved
cyclonedx/model/lifecycle.py Outdated Show resolved Hide resolved
tests/test_model_lifecycle.py Outdated Show resolved Hide resolved
cyclonedx/model/lifecycle.py Outdated Show resolved Hide resolved
cyclonedx/model/lifecycle.py Outdated Show resolved Hide resolved
cyclonedx/model/lifecycle.py Outdated Show resolved Hide resolved
Signed-off-by: Johannes Feichtner <johannes@web-wack.at>
@Churro
Copy link
Contributor Author

Churro commented Oct 10, 2024

I tried to align the class names as closely as possible with the CDX 1.5 specification to avoid potentially confusing homonyms.

I've also seen that cyclonedx-javascript-library uses LifecyclePhase, NamedLifecycle and cyclone-core-java uses LifecycleChoice. Related to these, would the following naming suggestion be ok for you?

  • Phase -> LifecyclePhase
  • CustomPhase -> NamedLifecycle
  • PredefinedPhase -> PredefinedLifecycle

@jkowalleck
Copy link
Member

I tried to align the class names as closely as possible with the CDX 1.5 specification to avoid potentially confusing homonyms.

I've also seen that cyclonedx-javascript-library uses LifecyclePhase, NamedLifecycle and cyclone-core-java uses LifecycleChoice. Related to these, would the following naming suggestion be ok for you?

* `Phase` -> `LifecyclePhase`

* `CustomPhase` -> `NamedLifecycle`

* `PredefinedPhase` -> `PredefinedLifecycle`

The renaming you proposed sounds reasonable. Please continue with that.

@jkowalleck jkowalleck self-requested a review October 10, 2024 18:14
Signed-off-by: Johannes Feichtner <johannes@web-wack.at>
…rmalize`

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
@jkowalleck
Copy link
Member

jkowalleck commented Oct 10, 2024

@Churro,

I've proposed some changes on top of yours: Churro#1
If you would simply merge my pull request, your pullrequest would update automatically.

PS: was done via df0fdf0

jkowalleck and others added 6 commits October 11, 2024 01:45
Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
@jkowalleck
Copy link
Member

jkowalleck commented Oct 11, 2024

@madpah could you review the current state of this PR? It aims to cover $.metadata.lifecycles from #578

@jkowalleck jkowalleck requested a review from a team October 11, 2024 11:20
@jkowalleck jkowalleck changed the title feat: add support for lifecycles in BOM metadata feat: add support for Lifecycles in BOM metadata Oct 15, 2024
@jkowalleck
Copy link
Member

@Churro,
Could you rebase onto the latest main branch and fix the conflicts accordingly?

Churro and others added 2 commits October 15, 2024 21:36
Signed-off-by: Johannes Feichtner <343448+Churro@users.noreply.github.com>
Signed-off-by: Johannes Feichtner <johannes@web-wack.at>
@jkowalleck
Copy link
Member

jkowalleck commented Oct 16, 2024

some of the tests are failing.

@Churro , you may run them locally as described in https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/CONTRIBUTING.md

PS: done so via bc74a86

Signed-off-by: Johannes Feichtner <johannes@web-wack.at>
@jkowalleck jkowalleck self-requested a review October 18, 2024 07:40
@jkowalleck jkowalleck requested review from madpah and a team October 18, 2024 07:46
@jkowalleck jkowalleck merged commit 6cfeb71 into CycloneDX:main Oct 21, 2024
41 checks passed
@jkowalleck
Copy link
Member

thank you for contributing this feature, @Churro .

it was released via v8.1.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request schema 1.5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants