Restructure pipelines for verbosity - #1074
Conversation
tdruez
left a comment
There was a problem hiding this comment.
@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
Reference: #1074 Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>
Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>
Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>
|
@tdruez thanks for the review comments! |
|
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. |
|
@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:
|
pombredanne
left a comment
There was a problem hiding this comment.
LGTM!
I posted a few nits regarding wordsmithing.
Please review, apply the good ones and this is good to merge.
51424c1 to
e9ebbd4
Compare
Suggested-by: Philippe Ombredanne <pombredanne@nexb.com> Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>
e9ebbd4 to
d92df4d
Compare
|
@pombredanne thanks for the suggestions, I've added them all with some more docstring updates! |
pombredanne
left a comment
There was a problem hiding this comment.
Thanks.... just one last nit, but nothing in the way of merging.
|
|
||
| .. _pipeline_scan_codebase_package: | ||
|
|
||
| Scan Codebase Package |
There was a problem hiding this comment.
Since this is renamed... we may still need an entry in the doc? This can be added later.
There was a problem hiding this comment.
@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.
|
@AyanSinhaMahapatra Merged, thanks! |
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