Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Restructure pipelines for verbosity #1074

Merged
merged 5 commits into from
Feb 14, 2024
Merged

Restructure pipelines for verbosity #1074

merged 5 commits into from
Feb 14, 2024

Conversation

AyanSinhaMahapatra
Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra commented Feb 9, 2024 •
edited
Loading

Remove scan_codebase_packages pipeline, and restructure inspect_packages pipeline into load_sbom and resolve_packages pipelines.

Reference: #1040
Reference: #1035
Reference: #1034
Reference: #1044

@AyanSinhaMahapatra
Restructure pipelines for verbosity
079d9e1 
Remove scan_codebase_packages pipeline, and restructure inspect_packages
pipeline into load_sbom and resolve_packages pipelines.

Reference: #1035
Reference: #1034
Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>
tdruez
tdruez requested changes Feb 11, 2024
View reviewed changes
Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

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

@AyanSinhaMahapatra see my various comments.

Also, the following is missing:

  • Unit tests
  • Release notes (very important considering the major pipeline renaming, changes, and additions)
  • The Built-in pipe docs need to be updated with those changes

scanpipe/pipelines/load_sbom.py Outdated Show resolved Hide resolved
scanpipe/pipelines/load_sbom.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
@AyanSinhaMahapatra
Refactor functions and improve docstrings
930b66c 
Reference: #1074
Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>
@AyanSinhaMahapatra
Add unittests for new functions
03c21a8 
Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>
@AyanSinhaMahapatra
Update docs and add CHANGELOG entry
b9da877 
Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>
@AyanSinhaMahapatra
Copy link
Member Author

@tdruez thanks for the review comments!
I've updated the PR to address all the issues mentioned above.
Didn't update the docs/changelog before as I wanted you and @pombredanne to check out the pipeline restructuring and approve, but I've updated them now, and this is ready for another round of review. Thanks!

@AyanSinhaMahapatra AyanSinhaMahapatra linked an issue Feb 12, 2024 that may be closed by this pull request
Inspect_manifest pipeline error for get_manifest_inputs() #1034
Closed
@AyanSinhaMahapatra
Copy link
Member Author

See also comment at #1035 (reply in thread), we've addressed all the comments there and are adhering to the naming convention now for all pipelines.

@tdruez
Copy link
Contributor

tdruez commented Feb 12, 2024

@AyanSinhaMahapatra Thanks for the changes. Everything looks fine on my side but I'd like to get @pombredanne review as well before merging as this is quite a big change:

  • 2 new pipelines
  • 1 removed pipeline

pombredanne
pombredanne requested changes Feb 13, 2024
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

LGTM!
I posted a few nits regarding wordsmithing.
Please review, apply the good ones and this is good to merge.

scanpipe/pipelines/load_sbom.py Outdated Show resolved Hide resolved
scanpipe/pipelines/load_sbom.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
scanpipe/pipelines/resolve_dependencies.py Outdated Show resolved Hide resolved
scanpipe/pipelines/scan_codebase.py Outdated Show resolved Hide resolved
scanpipe/pipelines/scan_single_package.py Outdated Show resolved Hide resolved
@AyanSinhaMahapatra AyanSinhaMahapatra force-pushed the restructure-pipelines branch 2 times, most recently from 51424c1 to e9ebbd4 Compare February 13, 2024 17:04
@AyanSinhaMahapatra
Improve docstrings for pipelines
d92df4d 
Suggested-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>
@AyanSinhaMahapatra AyanSinhaMahapatra linked an issue Feb 13, 2024 that may be closed by this pull request
Rename pipelines #1044
Closed
@AyanSinhaMahapatra
Copy link
Member Author

@pombredanne thanks for the suggestions, I've added them all with some more docstring updates!

pombredanne
pombredanne approved these changes Feb 13, 2024
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks.... just one last nit, but nothing in the way of merging.

docs/built-in-pipelines.rst
@@ -126,14 +142,6 @@ Scan Codebase
:members:
:member-order: bysource

.. _pipeline_scan_codebase_package:

Scan Codebase Package
Copy link
Member

Choose a reason for hiding this comment

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

Since this is renamed... we may still need an entry in the doc? This can be added later.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pombredanne we do have the new pipelines in the docs correctly, see: https://github.com/nexB/scancode.io/blob/restructure-pipelines/docs/built-in-pipelines.rst?plain=1#L61

Is this what you meant?

We are not having entries on renamed pipelines in the docs anymore: see https://github.com/nexB/scancode.io/pull/1053/files#diff-8a1644efb2ff73c9d7a85c18c73d2558528056d2fe7bb2adafe2502df72f4e2e also. The old pipelines are available only through the API/CLI for compatibility where they are renamed to the new pipelines. Otherwise we are not having the old pipeline names anywhere.

@tdruez tdruez merged commit 7bb4499 into main Feb 14, 2024
7 checks passed
@tdruez tdruez deleted the restructure-pipelines branch February 14, 2024 07:54
@tdruez
Copy link
Contributor

tdruez commented Feb 14, 2024

@AyanSinhaMahapatra Merged, thanks!

@AyanSinhaMahapatra AyanSinhaMahapatra mentioned this pull request Feb 22, 2024
Closed
@keshav-space keshav-space mentioned this pull request Feb 22, 2024
Closed
@AyanSinhaMahapatra AyanSinhaMahapatra mentioned this pull request Mar 25, 2024
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@pombredanne pombredanne pombredanne approved these changes

@tdruez tdruez Awaiting requested review from tdruez

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

Successfully merging this pull request may close these issues.

Rename pipelines Inspect_manifest pipeline error for get_manifest_inputs()
3 participants
@AyanSinhaMahapatra @tdruez @pombredanne