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: omit development dependencies from SBOM results #534

Merged
merged 11 commits into from
Mar 20, 2023

Conversation

tngraf
Copy link
Contributor

@tngraf tngraf commented Mar 15, 2023

Implements #474 in 4.x.x branch as discussed with @jkowalleck and @madpah

@tngraf tngraf requested a review from a team as a code owner March 15, 2023 13:47
@madpah madpah self-assigned this Mar 15, 2023
@madpah madpah added the enhancement New feature or request label Mar 15, 2023
@madpah madpah linked an issue Mar 15, 2023 that may be closed by this pull request
@madpah madpah added this to the 4.0.0 milestone Mar 15, 2023
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.

Thanks for your work, @tngraf.

There are critical bugs that need to be fixed. There are implementation details that need to be addressed.
These were marked with ❌ and ❗ and MUST be fixed.

I added other annotations which should be followed, or can be discussed.

cyclonedx_py/client.py Outdated Show resolved Hide resolved
@@ -235,6 +235,10 @@ def get_arg_parser(*, prog: Optional[str] = None) -> argparse.ArgumentParser:
'-pb', '--purl-bom-ref', action='store_true', dest='use_purl_bom_ref',
help="Use a component's PURL for the bom-ref value, instead of a random UUID"
)
arg_parser.add_argument(
"-omit", "--omit", dest="omit", action="append",
help="Omit specified items when using Poetry or PipEnv (currently supported is dev)",
Copy link
Member

Choose a reason for hiding this comment

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

❗ help text does not declare allowed values properly.
Let's mark this as an open topic and concentrate on the implementations first.

FYI: CLI and argparse might change to another implementation soon.
don't waste time now, we can discuss this at the feature finalization phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My assumption was that currently supported is dev is enough to get started.

Copy link
Member

Choose a reason for hiding this comment

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

lets leave this open.
if we go with Click8 then the help text generator will take over here.

Copy link
Member

Choose a reason for hiding this comment

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

we will fix this during the CLI/click8/subcomamnd rework

cyclonedx_py/parser/pipenv.py Outdated Show resolved Hide resolved
cyclonedx_py/parser/pipenv.py Outdated Show resolved Hide resolved
cyclonedx_py/parser/pipenv.py Outdated Show resolved Hide resolved
cyclonedx_py/parser/poetry.py Outdated Show resolved Hide resolved
cyclonedx_py/parser/pipenv.py Outdated Show resolved Hide resolved
cyclonedx_py/parser/poetry.py Outdated Show resolved Hide resolved
tests/fixtures/poetry-dev-dep.txt Show resolved Hide resolved
cyclonedx_py/parser/poetry.py Outdated Show resolved Hide resolved
Signed-off-by: Thomas Graf <[email protected]>
@jkowalleck
Copy link
Member

I do not like the implementation that the parsers do the filtering.
The job of the parsers is to generate the components and nothing more, right?
Filtering should be done after the parsers did the job.

read also: #474 (comment)

how do you see it?
@tngraf @madpah

README.md Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
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.

Couple of minor changes / updates mostly same as per @jkowalleck

@madpah
Copy link
Collaborator

madpah commented Mar 16, 2023

Agree @jkowalleck - but let's not complicate this addition with that bad brush. We can handle / refactor later...

@madpah madpah mentioned this pull request Mar 16, 2023
@jkowalleck
Copy link
Member

re: #534 (comment)

agree.
@madpah i hope we remember this :-)

@jkowalleck jkowalleck changed the title Feat: omit development dependencies from SBOM results in 4.x.x branch Feat: omit development dependencies from SBOM results Mar 17, 2023
@jkowalleck jkowalleck requested review from jkowalleck and madpah March 18, 2023 18:07
@jkowalleck
Copy link
Member

thanks for your work, @tngraf

LGTM.

i would love to have this merged, so I can draft some changes and refactoring,
and @madpah could drive the CLI/click8/subcommand/... rework.

@madpah madpah merged commit 1e27498 into CycloneDX:dev/4.x.x Mar 20, 2023
@jkowalleck jkowalleck mentioned this pull request Oct 25, 2023
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.

Feat: omit development dependencies from SBOM results
4 participants