Feat/dep track inspection #140
Conversation
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant DependencyTrackExporter
participant DependencyTrackAPI
User->>CLI: scanoss-py inspect dependency-track project-violations ...
CLI->>DependencyTrackExporter: Initialize with API URL, key, project ID
CLI->>DependencyTrackAPI: Request project processing status
DependencyTrackAPI-->>CLI: Return status (processing/complete)
CLI->>DependencyTrackAPI: Request project violations (when ready)
DependencyTrackAPI-->>CLI: Return violations data
CLI->>CLI: Sort and format violations (JSON/Markdown)
CLI->>User: Output results to file or stdout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
src/scanoss/inspection/raw/copyleft.py (1)
161-185: Method renamed appropriately but consider return type annotation.The method rename from
_filter_components_with_copyleft_licensesto_get_components_with_copyleft_licensesbetter reflects its purpose. However, the return type annotation showslist[Component]but the method processes raw dictionaries and returns them as-is.Consider either:
- Converting the dictionaries to
Componentdataclass instances before returning, or- Updating the return type annotation to reflect the actual return type (
list[dict])- def _get_components_with_copyleft_licenses(self, components: list) -> list[Component]: + def _get_components_with_copyleft_licenses(self, components: list) -> list[dict]:
🧹 Nitpick comments (6)
src/scanoss/inspection/raw/license_summary.py (1)
135-135: Consider using the existing debug logging mechanism.The hardcoded print statement could be replaced with the inherited debug logging capability for consistency.
- print("Running LicenseSummary") + self.print_debug("Running LicenseSummary")CLIENT_HELP.md (1)
370-374: Fix markdown list style inconsistency.The static analysis tools flagged inconsistent list markers. The project appears to use dashes for unordered lists, but these lines use asterisks.
-* Copyleft (`copyleft`) -* Undeclared Components (`undeclared`) -* License Summary (`license-summary`) -* Component Summary (`component-summary`) -* Dependency Track project violations (`dependency-track project-violations`) +- Copyleft (`copyleft`) +- Undeclared Components (`undeclared`) +- License Summary (`license-summary`) +- Component Summary (`component-summary`) +- Dependency Track project violations (`dependency-track project-violations`)src/scanoss/inspection/raw/undeclared_component.py (1)
75-75: Inconsistent parameter formattingThere's a missing space after the comma in the super().init call.
Apply this diff:
- debug, trace, quiet,format_type, filepath, output, status, name='Undeclared Components Policy' + debug, trace, quiet, format_type, filepath, output, status, name='Undeclared Components Policy'src/scanoss/inspection/dependency_track/project_violation.py (1)
160-173: Implement Jira markdown formattingThe
_jira_markdownmethod is not implemented but is required by the abstract base class.Would you like me to help implement the Jira markdown formatting based on the pattern used in other inspection classes?
src/scanoss/cli.py (2)
543-867: Well-structured hierarchical CLI design with good backward compatibility.The refactoring introduces a clean separation between raw scan results inspection and Dependency Track integration. The extensive documentation comments help understand the command structure. However, consider the following improvements:
The duplication of argument parsing logic between raw and legacy commands (lines 623-773) could be refactored into shared helper functions to reduce code duplication and maintenance burden.
The command aliases are consistent and intuitive (e.g.,
cpfor copyleft,dtfor dependency-track).
96-1164: Consider refactoring setup_args for better maintainability.The
setup_args()function has grown to over 1000 lines, which impacts readability and maintainability. Consider breaking it down into smaller functions:
- Extract inspect subcommand setup into a separate function like
setup_inspect_args(subparsers)- Extract common argument patterns into helper functions to reduce duplication
- Group related commands into separate setup functions
This would make the code more modular and easier to maintain as new features are added.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(2 hunks)CLIENT_HELP.md(2 hunks)src/scanoss/__init__.py(1 hunks)src/scanoss/cli.py(7 hunks)src/scanoss/export/dependency_track.py(3 hunks)src/scanoss/inspection/dependency_track/project_violation.py(1 hunks)src/scanoss/inspection/policy_check.py(4 hunks)src/scanoss/inspection/raw/component_summary.py(1 hunks)src/scanoss/inspection/raw/copyleft.py(7 hunks)src/scanoss/inspection/raw/license_summary.py(3 hunks)src/scanoss/inspection/raw/raw_base.py(3 hunks)src/scanoss/inspection/raw/undeclared_component.py(6 hunks)tests/test_policy_inspect.py(19 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/scanoss/export/dependency_track.py (2)
src/scanoss/cyclonedx.py (1)
CycloneDx(39-406)src/scanoss/scanossbase.py (1)
print_stdout(73-81)
src/scanoss/inspection/raw/license_summary.py (1)
src/scanoss/inspection/raw/raw_base.py (1)
RawBase(55-429)
src/scanoss/inspection/raw/raw_base.py (2)
src/scanoss/inspection/policy_check.py (1)
PolicyCheck(51-224)src/scanoss/inspection/utils/license_utils.py (1)
LicenseUtil(52-132)
src/scanoss/inspection/raw/component_summary.py (1)
src/scanoss/inspection/raw/raw_base.py (1)
RawBase(55-429)
src/scanoss/cli.py (6)
src/scanoss/inspection/dependency_track/project_violation.py (2)
DependencyTrackProjectViolationPolicyCheck(73-268)run(244-268)src/scanoss/inspection/raw/component_summary.py (2)
ComponentSummary(30-91)run(87-91)src/scanoss/inspection/raw/license_summary.py (2)
LicenseSummary(30-139)run(134-139)src/scanoss/inspection/raw/copyleft.py (2)
Copyleft(47-238)run(207-238)src/scanoss/inspection/raw/undeclared_component.py (2)
UndeclaredComponent(45-312)run(281-312)src/scanoss/scanossbase.py (1)
print_stderr(45-49)
🪛 GitHub Actions: Lint
src/scanoss/export/dependency_track.py
[error] 188-188: Ruff PLR2004: Magic value used in comparison, consider replacing 200 with a constant variable.
[error] 239-239: Ruff E501: Line too long (123 > 120).
src/scanoss/inspection/policy_check.py
[error] 25-29: Ruff I001: Import block is un-sorted or un-formatted. Organize imports.
src/scanoss/inspection/dependency_track/project_violation.py
[error] 1-7: Ruff I001: Import block is un-sorted or un-formatted. Organize imports.
[error] 210-210: Ruff E712: Avoid equality comparisons to True; use status['processing']: for truth checks.
src/scanoss/inspection/raw/copyleft.py
[error] 25-30: Ruff I001: Import block is un-sorted or un-formatted. Organize imports.
src/scanoss/inspection/raw/undeclared_component.py
[error] 25-30: Ruff I001: Import block is un-sorted or un-formatted. Organize imports.
🪛 markdownlint-cli2 (0.17.2)
CLIENT_HELP.md
370-370: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
371-371: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
372-372: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
373-373: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
374-374: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
🔇 Additional comments (29)
src/scanoss/__init__.py (1)
25-25: Version bump looks appropriate.The minor version increment from 1.30.0 to 1.31.0 correctly follows semantic versioning for the new
inspect dependency-track project-violationsfeature addition.src/scanoss/inspection/raw/component_summary.py (2)
27-27: Import change aligns with base class refactoring.The change from
InspectBasetoRawBaseis consistent with the systematic refactoring of raw inspection classes mentioned in the AI summary.
30-30: Base class inheritance updated correctly.The ComponentSummary class now properly inherits from
RawBase, which provides the necessary functionality for raw scan result inspection based on the relevant code snippets fromraw_base.py.CHANGELOG.md (2)
12-21: Changelog entries accurately document the changes.The version 1.31.0 entry correctly documents both the new
inspect dependency-track project-violationssubcommand and the renaming of existing inspect subcommands with the "raw" prefix. The date and format are consistent with project standards.
629-629: Version comparison link follows established pattern.The comparison link for version 1.31.0 is correctly formatted and follows the established pattern used throughout the changelog.
src/scanoss/inspection/raw/license_summary.py (3)
27-27: Import change consistent with base class refactoring.The change from
InspectBasetoRawBasefollows the same pattern as other raw inspection classes and is part of the systematic refactoring effort.
30-30: Class inheritance updated appropriately.The LicenseSummary class now correctly inherits from
RawBase, which provides the necessary functionality for processing raw scan results.
66-66: Constructor call properly adapted for RawBase.The explicit keyword arguments
filepath=filepath, output=outputcorrectly match the RawBase constructor signature shown in the relevant code snippets.CLIENT_HELP.md (3)
370-370: Typo correction is appropriate.The correction from "copylefet" to "copyleft" fixes the spelling error in the documentation.
374-374: New dependency-track feature properly documented.The addition of "Dependency Track project violations (
dependency-track project-violations)" correctly documents the new CLI subcommand introduced in this PR.
480-485: Example usage is comprehensive and helpful.The example command demonstrates all the required parameters for the new dependency-track subcommand, making it easy for users to understand how to use the feature.
src/scanoss/export/dependency_track.py (1)
33-33: LGTM!The change from absolute to relative import is appropriate for an internal module reference.
src/scanoss/inspection/raw/raw_base.py (2)
29-32: LGTM!The import changes correctly reflect the new class hierarchy and structure.
54-77: Well-structured generic base class refactoringThe refactoring to a generic base class
RawBase[T]with proper inheritance fromPolicyCheck[T]improves type safety and modularity. The constructor properly passes all required parameters to the superclass.src/scanoss/inspection/policy_check.py (2)
49-80: Excellent generic type implementationThe refactoring to use
Generic[T]with a type variable provides better type safety and flexibility. The constructor changes appropriately remove the filepath parameter which belongs in subclasses.
100-138: Consistent method signature updatesThe abstract method signatures have been consistently updated to use the generic type parameter
list[T], improving type safety across all implementations.src/scanoss/inspection/raw/undeclared_component.py (2)
32-44: Good use of dataclasses for type safetyThe introduction of
LicenseandComponentdataclasses provides clear type definitions and improves code maintainability.
298-312: Consistent variable naming updateThe renaming from
resultstodatamaintains consistency with the updated abstract method signatures in the base class.src/scanoss/inspection/dependency_track/project_violation.py (1)
10-72: Well-structured type definitionsThe use of TypedDict classes provides excellent type safety and documentation for the Dependency Track API response structures.
src/scanoss/inspection/raw/copyleft.py (4)
33-46: LGTM! Well-structured dataclasses improve type safety.The new
LicenseandComponentdataclasses provide clear, strongly-typed data structures that improve code maintainability and reduce the likelihood of runtime errors from incorrect field access.
47-47: LGTM! Generic inheritance aligns with refactoring.The change from
PolicyChecktoRawBase[Component]properly utilizes the new generic base class and provides better type safety for the component data structures.
90-106: LGTM! Type annotations improve method clarity.The updated method signature with
list[Component]type annotation provides better type safety and code documentation.
225-238: LGTM! Consistent variable naming improves readability.The change from
resultstodataaligns with the naming convention used across other inspection classes and makes the code more consistent.tests/test_policy_inspect.py (3)
33-37: LGTM! Import updates reflect the module refactoring correctly.The imports have been properly updated to use the new
rawsubpackage structure and include the newDependencyTrackProjectViolationPolicyCheckclass.
71-75: LGTM! Consistent variable naming improves test readability.The change from
resultstodatathroughout the test file aligns with the updated return convention from the inspection classes and maintains consistency across the codebase.
440-567: LGTM! Comprehensive test coverage for new Dependency Track functionality.The new test suite provides excellent coverage of the
DependencyTrackProjectViolationPolicyCheckclass:
- Tests both JSON and Markdown formatters
- Validates violation sorting by type (SECURITY, LICENSE, OTHER)
- Handles empty violation lists correctly
- Tests multiple violation types
- Verifies proper summary and detail formatting
The test data structure matches the expected Dependency Track API response format, and assertions are appropriate for validating the functionality.
src/scanoss/cli.py (3)
40-42: Well-organized modular import structure.The new imports properly reflect the modular refactoring of inspection classes into separate submodules under
scanoss.inspection.rawandscanoss.inspection.dependency_track.Also applies to: 73-74
1597-1836: Consistent and well-documented handler implementations.The inspection handler functions follow a consistent pattern with proper input validation, file handling, and exit codes. The comprehensive docstrings provide excellent documentation for each handler's purpose and parameters.
The handlers properly:
- Validate required inputs with clear error messages
- Initialize output files safely by clearing them first
- Pass all necessary configuration to the inspection classes
- Exit with the status returned by the inspection run
803-849: Comprehensive argument configuration for Dependency Track integration.The Dependency Track inspection arguments properly handle all necessary authentication and connection parameters. The format options are appropriately limited to
jsonandmdfor DT inspection, while raw inspections support additional formats likejira_md.Good use of descriptive help text for each argument to guide users.
agustingroh
force-pushed
the
feat/dep-track-inspection
branch
4 times, most recently
from
f7263ff to
571c587
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/scanoss/inspection/raw/copyleft.py (1)
169-182: Avoid mutating input data - work with copies instead.The method directly modifies the input components by deleting keys, which could cause unintended side effects if the same data is used elsewhere.
filtered_components = [] for component in components: copyleft_licenses = [lic for lic in component['licenses'] if lic['copyleft']] if copyleft_licenses: - # Remove unused keys - del component['count'] - del component['declared'] - del component['undeclared'] - filtered_component = component + # Create a copy of the component without unused keys + filtered_component = {k: v for k, v in component.items() + if k not in ['count', 'declared', 'undeclared']} # Remove 'count' from each license using pop - for lic in copyleft_licenses: - lic.pop('count', None) # None is default value if key doesn't exist + copyleft_licenses_clean = [] + for lic in copyleft_licenses: + lic_copy = lic.copy() + lic_copy.pop('count', None) + copyleft_licenses_clean.append(lic_copy) - filtered_component['licenses'] = copyleft_licenses + filtered_component['licenses'] = copyleft_licenses_clean filtered_components.append(filtered_component)
♻️ Duplicate comments (2)
src/scanoss/inspection/raw/copyleft.py (2)
26-30: Fix import sorting to resolve pipeline failure.The import block is still incorrectly sorted. Standard library imports should be grouped together at the top, followed by local imports.
import json +from dataclasses import dataclass +from typing import Any, Dict, List + -from dataclasses import dataclass -from typing import Any, Dict, List -from ..policy_check import PolicyStatus -from .raw_base import RawBase +from ..policy_check import PolicyStatus +from .raw_base import RawBase
80-80: Fix spacing in super() call.There's still inconsistent spacing in the super() call - remove the space before the comma.
- super().__init__(debug, trace, quiet, format_type,filepath, output ,status, name='Copyleft Policy') + super().__init__(debug, trace, quiet, format_type, filepath, output, status, name='Copyleft Policy')
🧹 Nitpick comments (1)
Dockerfile (1)
12-15: Consider pinning APT package versions for reproducible buildsUnversioned
apt-get installpulls the latest packages available at build time, which can produce different artefacts across days or mirrors and break CI caching. If reproducibility matters, specify package versions or use a snapshot repository (e.g.deb [arch=...] https://snapshot.debian.org/...) and install explicit versions:- && apt-get install -y --no-install-recommends build-essential gcc libicu-dev pkg-config \ + && apt-get install -y --no-install-recommends \ + build-essential=12.9 gcc=4:10.2.1-1 \ + libicu-dev=67.1-7 pkg-config=0.29.2-1 \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Dockerfile(1 hunks)src/scanoss/export/dependency_track.py(3 hunks)src/scanoss/inspection/dependency_track/project_violation.py(1 hunks)src/scanoss/inspection/policy_check.py(4 hunks)src/scanoss/inspection/raw/copyleft.py(7 hunks)src/scanoss/inspection/raw/undeclared_component.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/scanoss/inspection/policy_check.py
- src/scanoss/export/dependency_track.py
- src/scanoss/inspection/raw/undeclared_component.py
- src/scanoss/inspection/dependency_track/project_violation.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
Dockerfile (1)
12-15: Addition oflibicu-devandpkg-configis sound and isolated to the builder stageBoth libraries are frequently required to compile Unicode-aware C extensions (
libicu-dev) and to locate headers viapkg-config. Because the subsequent runtime stages are based onbase, these extra packages will not be present in the shipped images, so the size impact is confined to the builder layer.src/scanoss/inspection/raw/copyleft.py (4)
33-46: Well-structured dataclasses for type safety.The introduction of
LicenseandComponentdataclasses provides clear type definitions and improves code maintainability.
47-47: Good use of generic typing with RawBase.The change to inherit from
RawBase[Component]provides better type safety and aligns with the architectural improvements across the inspection modules.
90-90: Excellent type annotations and method naming.The explicit type annotations
list[Component]and the rename from_filter_to_get_better reflect the method's behavior and improve code clarity.Also applies to: 107-107, 134-134, 161-161
225-238: Consistent updates throughout the run method.All references properly updated to use the renamed method
_get_components_with_copyleft_licensesand the newdatavariable name, maintaining consistency with the refactoring pattern.
agustingroh
force-pushed
the
feat/dep-track-inspection
branch
from
a186f26 to
95d6011
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/scanoss/services/dependency_track_service.py (1)
25-27: Fix import sorting.The imports need to be properly sorted and formatted according to the pipeline failure.
src/scanoss/inspection/dependency_track/project_violation.py (1)
1-11: Fix import sorting.The imports need to be properly sorted and formatted according to the pipeline failure.
🧹 Nitpick comments (4)
src/scanoss/services/dependency_track_service.py (1)
45-49: Consider more robust validation methods.The validation methods use simple length checks which may not be sufficient for API keys and URLs.
Apply this diff to improve validation:
def _valid_key(self): - return len(self.api_key) != 0 + return self.api_key and len(self.api_key.strip()) > 0 def _valid_url(self): - return len(self.url) != 0 + return self.url and self.url.strip().startswith(('http://', 'https://'))src/scanoss/inspection/dependency_track/project_violation.py (3)
163-163: Fix line length violation.Line 163 exceeds the 120 character limit.
Apply this diff to fix the line length:
- "details": f"### Dependency Track Project Violations\n{self.generate_table(headers, rows, centered_columns)}\n", + "details": f"### Dependency Track Project Violations\n" + f"{self.generate_table(headers, rows, centered_columns)}\n",
115-118: Fix space formatting in constructor arguments.The constructor call has inconsistent spacing around arguments.
Apply this diff to fix the formatting:
- self.dependency_track_service = DependencyTrackService(dependency_track_api_key, - dependency_track_url,debug, - trace, - quiet) + self.dependency_track_service = DependencyTrackService( + dependency_track_api_key, + dependency_track_url, + debug, + trace, + quiet + )
167-180: Implement the _jira_markdown method.The method is defined but not implemented, which could cause runtime errors if called.
The
_jira_markdownmethod is currently not implemented. Would you like me to generate an implementation for Jira markdown formatting?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/scanoss/export/dependency_track.py(3 hunks)src/scanoss/inspection/dependency_track/project_violation.py(1 hunks)src/scanoss/services/dependency_track_service.py(1 hunks)tests/test_policy_inspect.py(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scanoss/export/dependency_track.py
🧰 Additional context used
🪛 GitHub Actions: Lint
src/scanoss/inspection/dependency_track/project_violation.py
[error] 1-1: I001 Import block is un-sorted or un-formatted. Organize imports.
[error] 163-163: E501 Line too long (124 > 120)
src/scanoss/services/dependency_track_service.py
[error] 25-25: I001 Import block is un-sorted or un-formatted. Organize imports.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
tests/test_policy_inspect.py (1)
439-610: Excellent test coverage for Dependency Track functionality.The new test suite comprehensively covers the
DependencyTrackProjectViolationPolicyCheckclass with proper mocking, edge cases, and multiple scenarios. The tests verify JSON/Markdown formatting, sorting logic, empty results handling, and multiple violation types.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/scanoss/inspection/dependency_track/project_violation.py (1)
8-16: Fix import sorting to resolve pipeline failureThe import statements need to be properly sorted according to the linter requirements.
Apply this diff to fix the import ordering:
import json import time from datetime import datetime -from typing import Any, Dict, List, Optional, TypedDict +from typing import Any, Dict, List, Optional, TypedDict + import requests from ..policy_check import PolicyCheck, PolicyStatus
🧹 Nitpick comments (1)
src/scanoss/inspection/dependency_track/project_violation.py (1)
178-180: Extract duplicate timestamp formatting logicThe timestamp formatting code is duplicated between methods, violating the DRY principle.
Consider extracting this to a helper method:
+ def _format_timestamp(self, timestamp: int) -> str: + """Format timestamp from milliseconds to human-readable string.""" + timestamp_seconds = timestamp / MILLISECONDS_TO_SECONDS + return datetime.fromtimestamp(timestamp_seconds).strftime("%d %b %Y at %H:%M:%S") + def _markdown(self, project_violations: list[PolicyViolationDict]) -> Dict[str, Any]: # ... existing code ... for project_violation in project_violations: - timestamp = project_violation['timestamp'] - timestamp_seconds = timestamp / MILLISECONDS_TO_SECONDS - formatted_date = datetime.fromtimestamp(timestamp_seconds).strftime("%d %b %Y at %H:%M:%S") + formatted_date = self._format_timestamp(project_violation['timestamp'])Also applies to: 217-219
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/scanoss/cli.py(7 hunks)src/scanoss/inspection/dependency_track/project_violation.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/scanoss/cli.py (6)
src/scanoss/inspection/dependency_track/project_violation.py (2)
DependencyTrackProjectViolationPolicyCheck(93-386)run(338-386)src/scanoss/inspection/raw/component_summary.py (2)
ComponentSummary(30-91)run(87-91)src/scanoss/inspection/raw/license_summary.py (2)
LicenseSummary(30-139)run(134-139)src/scanoss/inspection/raw/copyleft.py (2)
Copyleft(47-238)run(207-238)src/scanoss/inspection/raw/undeclared_component.py (2)
UndeclaredComponent(46-313)run(282-313)src/scanoss/scanossbase.py (1)
print_stderr(45-49)
🪛 GitHub Actions: Lint
src/scanoss/inspection/dependency_track/project_violation.py
[error] 8-16: I001 Import block is un-sorted or un-formatted. Organize imports.
[error] 192-192: E501 Line too long (124 > 120)
[error] 231-231: E501 Line too long (129 > 120)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
src/scanoss/inspection/dependency_track/project_violation.py (5)
24-92: Excellent type definitionsThe TypedDict definitions provide excellent type safety and documentation for the Dependency Track API responses. The comprehensive coverage of all response fields will help prevent runtime errors and improve code maintainability.
101-147: Well-structured initializationThe class initialization properly handles the complex configuration requirements while maintaining good documentation and clean parameter passing to parent and service classes.
235-317: Excellent error handling and separation of concernsThe core functionality methods demonstrate good practices:
- Proper null checking and exception handling in
_wait_project_processing- Clear validation logic in
_set_project_id- Clean separation of API calls and error handling
- Specific exception types with meaningful error messages
318-337: Clear and well-documented sorting logicThe static sorting method provides clear priority-based sorting with good documentation. The priority mapping is intuitive and the implementation is efficient.
338-386: Well-orchestrated main execution methodThe
runmethod demonstrates excellent architectural patterns:
- Clear execution flow with proper error boundaries
- Comprehensive trace logging for debugging
- Appropriate status code mapping for different outcomes
- Clean separation between data retrieval, processing, and output
src/scanoss/cli.py (4)
40-42: LGTM: Import restructuring aligns with new modular architecture.The imports have been correctly updated to use the new
rawsubmodule structure and include the new Dependency Track functionality. This maintains consistency with the hierarchical organization described in the PR.Also applies to: 73-74
543-883: Excellent hierarchical restructuring of inspect commands.The new command structure provides several benefits:
- Clear separation between raw scan analysis and external platform integration
- Backward compatibility maintained through legacy command preservation
- Comprehensive help text and argument descriptions
- Consistent argument patterns across command groups
The implementation properly handles the complex nested subcommand structure while maintaining usability.
1609-1858: Well-implemented command handlers with excellent practices.The new inspection command handlers demonstrate several best practices:
- Comprehensive input validation with helpful error messages
- Consistent file handling patterns (create/clear before use)
- Proper parameter validation and error reporting
- Detailed docstrings explaining functionality and parameters
- Appropriate exit codes for different failure scenarios
- Clean separation between validation, setup, and execution
The error handling is particularly well done, providing clear feedback to users when required parameters are missing.
1144-1152: Correct addition of debug/trace options to new inspect commands.The new inspect command parsers have been properly added to the debug/trace/quiet options list, ensuring consistent behavior across all inspection commands. This follows the established pattern used throughout the CLI.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/scanoss/services/dependency_track_service.py (2)
98-124: Missing validation as noted in past review.The method lacks validation of API credentials before making the request, unlike other methods in the class.
This issue was previously identified and still needs to be addressed. Add validation at the start of the method:
def get_project_status(self, upload_token): """ Get Dependency Track project processing status. Queries the Dependency Track API to check if the project upload processing is complete using the upload token. Returns: dict: Project status information or None if request fails """ + + if not self._valid_url(): + raise Exception("Invalid Dependency Track URL") + + if not self._valid_key(): + raise Exception("Invalid key") + + if not upload_token: + raise Exception("Invalid upload token") if self.trace: self.print_trace(f'URL: {self.url}') self.print_trace(f'Upload token: {upload_token}')
125-139: Inconsistent error return type as noted in past review.The method returns the exception object on failure but returns a list/dict on success, causing inconsistent return types.
This issue was previously identified and still needs to be addressed:
def get_project_violations(self,project_id:str): """ Get project violations from Dependency Track. Returns: list: List of policy violations or None if request fails """ url = f"{self.url}/api/v1/violation/project/{project_id}" req_headers = {'X-Api-Key': self.api_key, 'Content-Type': 'application/json'} try: response = requests.get(url, headers=req_headers) response.raise_for_status() # Raises an HTTPError for bad responses return response.json() except requests.exceptions.RequestException as e: self.print_stderr(f"Error getting Dependency Track project violations: {e}") - return e + return NoneAdditionally, this method should include validation like the other methods:
def get_project_violations(self,project_id:str): """ Get project violations from Dependency Track. Returns: list: List of policy violations or None if request fails """ + + if not self._valid_url(): + raise Exception("Invalid Dependency Track URL") + + if not self._valid_key(): + raise Exception("Invalid key") + + if not project_id: + raise Exception("Invalid project ID") + url = f"{self.url}/api/v1/violation/project/{project_id}" req_headers = {'X-Api-Key': self.api_key, 'Content-Type': 'application/json'}
🧹 Nitpick comments (1)
src/scanoss/services/dependency_track_service.py (1)
45-49: Consider improving validation methods.The validation methods are overly simplistic and could be enhanced for better robustness.
Apply this diff to improve validation:
def _valid_key(self): - return len(self.api_key) != 0 + return self.api_key and self.api_key.strip() def _valid_url(self): - return len(self.url) != 0 + return self.url and self.url.strip() and (self.url.startswith('http://') or self.url.startswith('https://'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/scanoss/export/dependency_track.py(3 hunks)src/scanoss/inspection/dependency_track/project_violation.py(1 hunks)src/scanoss/services/dependency_track_service.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/scanoss/export/dependency_track.py
- src/scanoss/inspection/dependency_track/project_violation.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/scanoss/services/dependency_track_service.py (2)
src/scanoss/scanossbase.py (2)
ScanossBase(28-107)print_trace(65-70)src/scanoss/cli.py (1)
export_dt(1491-1520)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/scanoss/services/dependency_track_service.py (2)
1-44: LGTM! Clean class structure and initialization.The class properly extends
ScanossBaseand follows established patterns. The URL trimming in the constructor is a good practice to handle trailing slashes consistently.
51-96: LGTM! Well-structured method with proper validation.The method includes comprehensive validation and error handling. The tracing functionality is well implemented for debugging purposes.
agustingroh
force-pushed
the
feat/dep-track-inspection
branch
from
82ea418 to
0393afd
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
agustingroh
force-pushed
the
feat/dep-track-inspection
branch
from
0393afd to
2932c07
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/test_policy_inspect.py (1)
70-74: Verify test expectation aligns with Copyleft implementation.Based on past review comments, there was confusion about whether
Copyleft.run()should returnPOLICY_SUCCESSorPOLICY_FAILwhen no copyleft licenses are found. The current test expectsPOLICY_SUCCESS.value, which appears to be correct according to the implementation in the relevant code snippets - when no components are found, it should be considered successful (no policy violations).src/scanoss/inspection/dependency_track/project_violation.py (2)
272-294: Fix missing return statement in _wait_processing_by_project_status.The method performs processing checks but doesn't return the status, making it ineffective for the caller.
Apply this diff to return the status:
def _wait_processing_by_project_status(self): """ Wait for project processing to complete in Dependency Track. Returns: Project status dictionary or None if processing fails or times out """ start_time = time.time() while True: status = self.dep_track_service.get_project_status(self.upload_token) if status is None: self.print_stderr(f'Error getting project status for upload token: {self.upload_token}') - break + return None if status and not status.get('processing'): self.print_debug(f'Project Status: {status}') - break + return status if time.time() - start_time > self.timeout: self.print_msg(f'Timeout reached ({self.timeout}s) while waiting for project processing') - break + return status time.sleep(PROCESSING_RETRY_DELAY) self.print_debug('Checking if complete...') - # End while loop
295-307: Fix flawed logic in _wait_project_processing method.The method always calls
_wait_processing_by_project_idregardless of whetherupload_tokenis provided, and doesn't use the result from_wait_processing_by_project_status.Apply this diff to fix the logic:
def _wait_project_processing(self): """ Wait for project processing to complete in Dependency Track. Returns: Project status dictionary or None if processing fails """ if self.upload_token: self.print_debug("Using upload token to check project status") - self._wait_processing_by_project_status() - self.print_debug("Using project id to get project status") - return self._wait_processing_by_project_id() + return self._wait_processing_by_project_status() + else: + self.print_debug("Using project id to get project status") + return self._wait_processing_by_project_id()
🧹 Nitpick comments (2)
src/scanoss/inspection/dependency_track/project_violation.py (2)
399-413: Address incomplete docstring and remove unnecessary TODO.The docstring for the
runmethod is incomplete (ends with "???") and contains a TODO that questions the return type design.Apply this diff to fix the documentation:
- # TODO Why return a tuple? Doesn't look like it's used anywhere? def run(self) -> tuple[int, Optional[Dict[str, Any]]]: """ Execute the policy check for Dependency Track project violations. Retrieves project violations from Dependency Track, sorts them by priority, formats the output according to the specified format, and outputs the results. Returns: Tuple of (status_code, formatted_data) where status_code indicates: - SUCCESS if violations are found, FAIL if no violations, ERROR if failed + POLICY_SUCCESS if no violations found, POLICY_FAIL if violations exist, ERROR if failed + Raises: - ValueError: If the project is still processing violations or if the??? - ????? + ValueError: If the project name/version is missing or project not found """
421-421: Potential security issue with API key logging.Line 421 logs the API key length, which could leak information about the key. While the key itself is masked, the length could be sensitive information.
Consider removing the API key logging entirely or use a generic placeholder:
- self.print_msg(f'API Key: {"*" * len(self.api_key)}') + self.print_msg('API Key: [REDACTED]')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/scanoss/inspection/dependency_track/project_violation.py(1 hunks)src/scanoss/inspection/policy_check.py(5 hunks)src/scanoss/inspection/raw/copyleft.py(6 hunks)tests/test_policy_inspect.py(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/scanoss/inspection/raw/copyleft.py
- src/scanoss/inspection/policy_check.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_policy_inspect.py (4)
src/scanoss/inspection/policy_check.py (4)
PolicyStatus(33-44)run(83-98)_json(101-112)_markdown(115-125)src/scanoss/inspection/raw/copyleft.py (4)
Copyleft(47-233)run(213-233)_json(90-105)_markdown(107-114)src/scanoss/inspection/raw/undeclared_component.py (4)
UndeclaredComponent(46-304)run(282-304)_json(165-180)_markdown(182-198)src/scanoss/inspection/dependency_track/project_violation.py (5)
DependencyTrackProjectViolationPolicyCheck(119-451)run(400-451)_json(171-184)_markdown(186-196)_sort_project_violations(334-351)
🪛 GitHub Actions: Lint
src/scanoss/inspection/dependency_track/project_violation.py
[error] 31-36: Ruff: Module level import not at top of file (E402). Multiple import statements from lines 31 to 36 are misplaced. Also, import block is unsorted or unformatted (I001). 7 errors found, 1 fixable with --fix option.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
tests/test_policy_inspect.py (2)
29-36: LGTM: Import reorganization reflects new module structure.The imports have been properly updated to reflect the refactored module structure with raw inspection classes moved to
src.scanoss.inspection.rawsubpackage and the new Dependency Track integration.
439-608: Comprehensive test coverage for DependencyTrackProjectViolationPolicyCheck.The new test suite provides excellent coverage of the DependencyTrackProjectViolationPolicyCheck functionality:
- JSON and Markdown formatting (
test_dependency_track_project_violation_json_formatter,test_dependency_track_project_violation_markdown_formatter)- Violation sorting by priority (
test_dependency_track_project_violation_sort_violations)- Empty violations handling (
test_dependency_track_project_violation_empty_violations,test_dependency_track_project_violation_markdown_empty)- Multiple violation types (
test_dependency_track_project_violation_multiple_types)The tests properly mock the
DependencyTrackServicedependency and verify expected outputs. The test data structure matches thePolicyViolationDictTypedDict definition.src/scanoss/inspection/dependency_track/project_violation.py (1)
44-117: LGTM: Well-structured TypedDict definitions.The TypedDict definitions provide excellent type safety for Dependency Track API entities. The hierarchical structure with
ResolvedLicenseDict,ProjectDict,ComponentDict,PolicyDict,PolicyConditionDict, andPolicyViolationDictproperly models the API response structure.
| import json | ||
| import time | ||
| from datetime import datetime | ||
| from typing import Any, Dict, List, Optional, TypedDict | ||
| from ...services.dependency_track_service import DependencyTrackService | ||
| from ..policy_check import PolicyCheck, PolicyStatus |
There was a problem hiding this comment.
Fix import ordering to resolve pipeline failures.
The imports are positioned after the module docstring but the linter requires them to be organized differently. The pipeline failures indicate both placement (E402) and sorting (I001) issues.
Apply this diff to fix the import issues:
-import json
-import time
-from datetime import datetime
-from typing import Any, Dict, List, Optional, TypedDict
-from ...services.dependency_track_service import DependencyTrackService
-from ..policy_check import PolicyCheck, PolicyStatus
+import json
+import time
+from datetime import datetime
+from typing import Any, Dict, List, Optional, TypedDict
+
+from ..policy_check import PolicyCheck, PolicyStatus
+from ...services.dependency_track_service import DependencyTrackService📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import json | |
| import time | |
| from datetime import datetime | |
| from typing import Any, Dict, List, Optional, TypedDict | |
| from ...services.dependency_track_service import DependencyTrackService | |
| from ..policy_check import PolicyCheck, PolicyStatus | |
| import json | |
| import time | |
| from datetime import datetime | |
| from typing import Any, Dict, List, Optional, TypedDict | |
| from ..policy_check import PolicyCheck, PolicyStatus | |
| from ...services.dependency_track_service import DependencyTrackService |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 31-36: Ruff: Module level import not at top of file (E402). Multiple import statements from lines 31 to 36 are misplaced. Also, import block is unsorted or unformatted (I001). 7 errors found, 1 fixable with --fix option.
🤖 Prompt for AI Agents
In src/scanoss/inspection/dependency_track/project_violation.py around lines 31
to 36, the import statements are incorrectly ordered and placed after the module
docstring, causing linter errors E402 and I001. Move all import statements to
immediately follow the module docstring, then reorder them according to PEP8
guidelines: standard library imports first, followed by third-party imports, and
finally local application imports, each group sorted alphabetically. This will
fix the import placement and sorting issues causing pipeline failures.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/scanoss/inspection/dependency_track/project_violation.py (2)
297-309: Fix flawed conditional logic.The method always executes
_wait_processing_by_project_id()and ignores the result from_wait_processing_by_project_status()when upload_token is available.Apply this diff to fix the logic:
def _wait_project_processing(self): """ Wait for project processing to complete in Dependency Track. Returns: Project status dictionary or None if processing fails """ if self.upload_token: self.print_debug("Using upload token to check project status") - self._wait_processing_by_project_status() - self.print_debug("Using project id to get project status") - return self._wait_processing_by_project_id() + return self._wait_processing_by_project_status() + else: + self.print_debug("Using project id to get project status") + return self._wait_processing_by_project_id()
274-296: Fix missing return statements.The method breaks out of the loop but doesn't return the status value, making it ineffective for the caller.
Apply this diff to return the status properly:
def _wait_processing_by_project_status(self): """ Wait for project processing to complete in Dependency Track. Returns: Project status dictionary or None if processing fails or times out """ start_time = time.time() while True: status = self.dep_track_service.get_project_status(self.upload_token) if status is None: self.print_stderr(f'Error getting project status for upload token: {self.upload_token}') - break + return None if status and not status.get('processing'): self.print_debug(f'Project Status: {status}') - break + return status if time.time() - start_time > self.timeout: self.print_msg(f'Timeout reached ({self.timeout}s) while waiting for project processing') - break + return status time.sleep(PROCESSING_RETRY_DELAY) self.print_debug('Checking if complete...') - # End while loop
🧹 Nitpick comments (2)
src/scanoss/inspection/dependency_track/project_violation.py (2)
401-401: Address TODO comment about return tuple.The comment questions why a tuple is returned and whether it's used. Consider reviewing the caller code to determine if the return type can be simplified.
Would you like me to search the codebase to see how this return value is used?
#!/bin/bash # Search for usage of DependencyTrackProjectViolationPolicyCheck.run method ast-grep --pattern 'class $_ { $$$ def run($$$) -> tuple[$_, $_]: $$$ }' # Also search for any calls to .run() method rg -A 3 -B 3 '\.run\(\)'
413-415: Fix incomplete docstring.The docstring has incomplete text with question marks that should be completed or removed.
Apply this diff to fix the docstring:
Returns: Tuple of (status_code, formatted_data) where status_code indicates: SUCCESS if violations are found, FAIL if no violations, ERROR if failed - Raises: - ValueError: If the project is still processing violations or if the??? - ?????
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/scanoss/inspection/dependency_track/project_violation.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
src/scanoss/inspection/dependency_track/project_violation.py (9)
24-30: Imports are properly organized now.The imports are correctly positioned after the module docstring and properly sorted according to PEP8 guidelines (standard library first, then local imports with proper grouping).
46-119: Excellent use of TypedDict for API type safety.The comprehensive TypedDict definitions provide strong type safety for Dependency Track API responses and improve code maintainability.
129-172: Well-structured constructor with comprehensive parameter handling.The initialization properly handles all necessary configuration parameters and correctly instantiates the dependency service.
173-211: Clean formatting methods with proper separation of concerns.The formatting methods are well-structured with consistent interfaces and appropriate delegation patterns.
212-248: Robust project completion detection logic.The method properly handles edge cases and provides comprehensive validation of project processing status with good debug output.
249-273: Proper polling implementation with timeout handling.The method correctly implements a polling pattern with appropriate timeout and error handling mechanisms.
310-334: Solid project ID resolution logic.The method properly handles both direct ID specification and name/version lookup with appropriate error handling.
335-354: Efficient sorting implementation.The static method correctly prioritizes violations by type (SECURITY > LICENSE > OTHER) using a clean sorting approach.
451-453: Verify policy status return logic.The method returns
POLICY_FAILwhen violations are found andPOLICY_SUCCESSwhen no violations exist. This logic seems correct for a policy checker - violations indicate policy failure.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/scanoss/inspection/dependency_track/project_violation.py (3)
24-30: Fix import ordering to resolve pipeline failures.The pipeline indicates import ordering violations (E402, I001). The imports need to be properly organized according to PEP8 guidelines.
Apply this diff to fix the import ordering:
+import json +import time +from datetime import datetime +from typing import Any, Dict, List, Optional, TypedDict + +from ...services.dependency_track_service import DependencyTrackService +from ..policy_check import PolicyCheck, PolicyStatusEnsure standard library imports come first (json, time, datetime, typing), followed by a blank line, then local imports sorted alphabetically.
274-295: Fix missing return statements in _wait_processing_by_project_status.The method performs processing checks but doesn't return the status, making it ineffective for the caller as noted in past reviews.
Apply this diff to return the status:
def _wait_processing_by_project_status(self): """ Wait for project processing to complete in Dependency Track. Returns: Project status dictionary or None if processing fails or times out """ start_time = time.time() while True: status = self.dep_track_service.get_project_status(self.upload_token) if status is None: self.print_stderr(f'Error getting project status for upload token: {self.upload_token}') - break + return None if status and not status.get('processing'): self.print_debug(f'Project Status: {status}') - break + return status if time.time() - start_time > self.timeout: self.print_msg(f'Timeout reached ({self.timeout}s) while waiting for project processing') - break + return status time.sleep(PROCESSING_RETRY_DELAY) self.print_debug('Checking if complete...') - # End while loop
297-308: Fix flawed logic in _wait_project_processing method.The method always calls
_wait_processing_by_project_idregardless of whetherupload_tokenis provided, and doesn't use the result from_wait_processing_by_project_status.Apply this diff to fix the logic:
def _wait_project_processing(self): """ Wait for project processing to complete in Dependency Track. Returns: Project status dictionary or None if processing fails """ if self.upload_token: self.print_debug("Using upload token to check project status") - self._wait_processing_by_project_status() - self.print_debug("Using project id to get project status") - return self._wait_processing_by_project_id() + return self._wait_processing_by_project_status() + else: + self.print_debug("Using project id to get project status") + return self._wait_processing_by_project_id()src/scanoss/cli.py (2)
1553-1816: Excellent improvement in handler function design and documentationThe handler functions demonstrate significant improvements with comprehensive docstrings, consistent error handling, and proper input validation. The integration with the inspection classes follows good patterns.
Issues to address:
File creation pattern: The
initialise_empty_file()function still usesopen(file, 'w').close()(line 2508) which should be replaced withPath(file).touch()as noted in previous reviews.Error handling specificity: Consider catching more specific exceptions where possible instead of generic
Exceptionin the handler functions.The overall structure and documentation quality represent a significant improvement in maintainability and user experience.
2490-2511: Good consolidation but file creation pattern still needs improvementThe
initialise_empty_filehelper function is a good step toward consistent file handling, and the error handling is properly implemented. However, it still uses theopen(file, 'w').close()pattern that should be replaced.Consider using
Path.touch()for more robust file creation:def initialise_empty_file(filename: str): if filename: try: - open(filename, 'w').close() + Path(filename).touch() except OSError as e: print_stderr(f'Error: Unable to create output file {filename}: {e}') sys.exit(1)This approach is more explicit about its intent and handles edge cases better.
🧹 Nitpick comments (1)
src/scanoss/export/dependency_track.py (1)
37-55: Improve parameter documentation in docstring.The function logic is sound, but the docstring should document all parameters for better clarity.
Apply this diff to improve the docstring:
def _build_payload(encoded_sbom: str, project_id, project_name, project_version) -> dict: """ Build the API payload Args: encoded_sbom: Base64 encoded SBOM + project_id: Project ID (optional, takes precedence over name/version) + project_name: Project name (used when project_id is None) + project_version: Project version (used when project_id is None) Returns: API payload dictionary """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/scanoss/cli.py(14 hunks)src/scanoss/export/dependency_track.py(3 hunks)src/scanoss/inspection/dependency_track/project_violation.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint
src/scanoss/export/dependency_track.py
[error] 62-62: Ruff: Too many arguments in function definition (6 > 5) (PLR0913)
[error] 153-153: Ruff: Too many branches in function (13 > 12) (PLR0912)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (26)
src/scanoss/export/dependency_track.py (5)
88-108: LGTM!The validation logic is well-structured with proper error handling and clear error messages. The method correctly validates both JSON format and CycloneDX SBOM structure.
110-124: LGTM!Good defensive programming with the empty content warning. The base64 encoding logic is correct and handles the JSON serialization properly.
126-151: LGTM!Good separation of concerns with the method delegating file processing to
upload_sbom_contents. Error handling is appropriate and the method follows a clear flow.
215-223: LGTM! Good error handling improvements.The exception handling is comprehensive, covering ValueError, RequestException, and general exceptions. Error messages are properly routed to stderr, and debug mode provides stack traces for troubleshooting.
196-208: LGTM! Good token and UUID processing logic.The logic correctly handles token extraction and project UUID resolution with appropriate fallbacks. The use of the DependencyTrackService for UUID lookup is well-integrated, and the JSON output formatting is clean.
src/scanoss/inspection/dependency_track/project_violation.py (10)
46-119: LGTM! Excellent use of TypedDict for API type safety.The comprehensive TypedDict definitions provide strong typing for Dependency Track API entities. This improves code clarity, IDE support, and helps catch type-related issues early.
129-171: LGTM! Constructor complexity is justified.While the constructor has many parameters, they're all necessary for the policy check functionality. The
noqa: PLR0913comment appropriately acknowledges this complexity, and the initialization logic is well-structured.
173-186: LGTM! Clean JSON formatting implementation.The method provides clean JSON formatting with appropriate indentation and a summary count. The structure is consistent with the expected output format.
188-210: LGTM! Good use of DRY principle.The methods effectively use the
_md_summary_generatorhelper to avoid code duplication while supporting different table formats (standard Markdown vs Jira Markdown).
212-247: LGTM! Robust project status checking logic.The method implements comprehensive logic to determine if a project has finished processing by comparing multiple timestamps. The validation checks for zero values and the debug output aids troubleshooting. The logic requiring both vulnerability analysis AND metrics to be current is appropriate.
249-272: LGTM! Well-implemented polling logic.The method implements proper polling with timeout handling, retry delays, and appropriate error messages. The logic correctly returns the project when processing is complete or provides timeout warnings.
310-333: LGTM! Solid project ID resolution logic.The method implements proper project ID resolution with comprehensive error handling and validation. The error messages are clear and the fallback logic from name/version to UUID is well-implemented.
335-353: LGTM! Good violation sorting logic.The method implements logical priority sorting with SECURITY violations first, followed by LICENSE, then OTHER types. The lambda-based sorting approach is clean and efficient.
355-399: LGTM! Comprehensive Markdown generation.The method handles null violations gracefully and generates well-formatted Markdown tables. The timestamp conversion and component formatting are appropriate. The use of a configurable table generator promotes code reuse.
401-443: LGTM! Well-orchestrated workflow with correct policy status logic.The method properly orchestrates the complete workflow: setting project ID, waiting for processing, retrieving violations, and formatting output. The return logic correctly returns
POLICY_FAILwhen violations exist andPOLICY_SUCCESSwhen no violations are found, which aligns with policy check semantics.src/scanoss/cli.py (11)
36-40: LGTM - Import restructuring aligns with modular designThe updated imports correctly reflect the new hierarchical structure with raw inspection classes and the addition of Dependency Track functionality. This supports better code organization and separation of concerns.
Also applies to: 70-71
540-826: Excellent hierarchical CLI restructuring with backward compatibilityThe new inspect command structure demonstrates excellent design with:
- Clear separation between raw results, legacy commands, and Dependency Track functionality
- Comprehensive documentation comments that improve maintainability
- Efficient argument sharing between raw and legacy parsers to avoid duplication
- Proper backward compatibility preservation for existing users
- Well-organized sub-command hierarchy that scales effectively
The extensive comments and consistent naming conventions make the code highly maintainable.
1088-1096: LGTM - Consistent debug options across all inspection commandsThe addition of debug, trace, and quiet options to all new inspection parsers ensures consistent CLI behavior and debugging capabilities across the entire inspect command hierarchy.
1123-1125: LGTM - Proper handling of three-level command hierarchyThe updated argument parsing correctly handles the new hierarchical structure and ensures appropriate help messages are displayed for incomplete command paths in the inspect sub-commands.
1160-1160: LGTM - Consistent file initialization and parameter alignmentGood use of the new
initialise_empty_file()utility for consistent error handling, and the parameter name update properly aligns with the FileCount class interface.Also applies to: 1166-1166
1196-1196: LGTM - Consistent file handling and method parameter updatesThe changes properly use the new file initialization utility and correctly pass output parameters to the scanner methods, maintaining consistency with the updated interface.
Also applies to: 1229-1229, 1235-1235, 1237-1237
1335-1335: LGTM - Consistent patterns applied to scan functionThe changes follow the established patterns of using the file initialization utility and updating parameter names to match the Scanner class interface.
Also applies to: 1390-1390
1503-1503: LGTM - Consistent updates with explicit returnGood use of the file initialization utility and parameter name alignment with the ScancodeDeps interface. The explicit
return Noneimproves code clarity.Also applies to: 1508-1508, 1510-1510
1818-1853: LGTM - Comprehensive improvements to export functionThe updated
export_dtfunction shows excellent improvements:
- Comprehensive docstring with clear parameter documentation
- Proper argument validation using the helper function
- Consistent use of file initialization utility
- Parameter names aligned with the updated DependencyTrackExporter interface
1855-1883: Excellent extraction of validation logicThe
_dt_args_validatorfunction properly centralizes Dependency Track argument validation with:
- Clear validation logic for project identification requirements
- Comprehensive docstring explaining the validation rules
- Helpful error messages guiding users to correct usage
- Good reusability between export and inspect commands
2487-2488: LGTM - Improved readability with spacingThe added spacing improves code readability and separation between functions.
| def __init__( | ||
| self, | ||
| config: DependencyTrackExporterConfig, | ||
| url: str = None, | ||
| apikey: str = None, | ||
| output: str = None, | ||
| debug: bool = False, | ||
| trace: bool = False, | ||
| quiet: bool = False, | ||
| quiet: bool = False | ||
| ): | ||
| """ | ||
| Initialize DependencyTrackExporter | ||
|
|
||
| Args: | ||
| config: Configuration parameters for the dependency track exporter | ||
| url: Dependency Track URL | ||
| apikey: Dependency Track API Key | ||
| output: File to store output response data (optional) | ||
| debug: Enable debug output | ||
| trace: Enable trace output | ||
| quiet: Enable quiet mode | ||
| """ | ||
| super().__init__(debug=debug, trace=trace, quiet=quiet) | ||
|
|
||
| self.dt_url = config.dt_url.rstrip('/') | ||
| self.dt_apikey = config.dt_apikey | ||
| self.dt_projectid = config.dt_projectid | ||
| self.dt_projectname = config.dt_projectname | ||
| self.dt_projectversion = config.dt_projectversion | ||
|
|
||
| self._validate_config() | ||
|
|
||
| def _validate_config(self): | ||
| """ | ||
| Validate that the configuration is valid. | ||
| """ | ||
| has_id = bool(self.dt_projectid) | ||
| has_name_version = bool(self.dt_projectname and self.dt_projectversion) | ||
|
|
||
| if not (has_id or has_name_version): | ||
| raise ValueError('Either --dt-projectid OR (--dt-projectname and --dt-projectversion) must be provided') | ||
|
|
||
| if has_id and has_name_version: | ||
| self.print_debug('Both DT project ID and name/version provided. Using project ID.') | ||
| self.url = url.rstrip('/') | ||
| self.apikey = apikey | ||
| self.output = output | ||
| self.dt_service = DependencyTrackService(self.apikey, self.url, debug=debug, trace=trace, quiet=quiet) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce constructor complexity to address pipeline failure.
The constructor has 6 parameters, exceeding the recommended limit of 5. Consider refactoring to reduce complexity.
Option 1: Group related parameters into a configuration object
Create a configuration dataclass:
from dataclasses import dataclass
@dataclass
class DependencyTrackConfig:
url: str
apikey: str
output: str = None
debug: bool = False
trace: bool = False
quiet: bool = FalseThen modify the constructor:
- def __init__(
- self,
- url: str = None,
- apikey: str = None,
- output: str = None,
- debug: bool = False,
- trace: bool = False,
- quiet: bool = False
- ):
+ def __init__(self, config: DependencyTrackConfig):**Option 2: Use kwargs for optional parameters
def __init__(
self,
url: str = None,
apikey: str = None,
- output: str = None,
- debug: bool = False,
- trace: bool = False,
- quiet: bool = False
+ **kwargs
):🧰 Tools
🪛 GitHub Actions: Lint
[error] 62-62: Ruff: Too many arguments in function definition (6 > 5) (PLR0913)
🤖 Prompt for AI Agents
In src/scanoss/export/dependency_track.py lines 62 to 86, the constructor has
six parameters, exceeding the recommended limit of five. To fix this, refactor
the constructor by grouping related parameters into a configuration dataclass
that holds url, apikey, output, debug, trace, and quiet attributes, then modify
the constructor to accept an instance of this config class instead of individual
parameters. Alternatively, use **kwargs to handle optional parameters like
debug, trace, and quiet, reducing the explicit parameter count in the
constructor signature.
| def upload_sbom_contents(self, sbom_content: dict, project_id, project_name, project_version, output_file) -> bool: | ||
| """ | ||
| Uploads an SBOM to a Dependency Track server. | ||
|
|
||
| payload = self._build_payload(encoded_sbom) | ||
| Parameters: | ||
| sbom_content (dict): The SBOM content in dictionary format to be uploaded. | ||
| project_id: The unique identifier for the project. | ||
| project_name: The name of the project in Dependency Track. | ||
| project_version: The version of the project in Dependency Track. | ||
| output_file: The path to the file where the token and UUID data | ||
| should be written. If not provided, the data will be written to | ||
| standard output. | ||
|
|
||
| url = f'{self.dt_url}/api/v1/bom' | ||
| headers = {'Content-Type': 'application/json', 'X-Api-Key': self.dt_apikey} | ||
| Returns: | ||
| bool: True if the upload is successful; False otherwise. | ||
|
|
||
| Raises: | ||
| ValueError: If the SBOM encoding process fails. | ||
| requests.exceptions.RequestException: If an error occurs during the HTTP request. | ||
| Exception: For any other unexpected error. | ||
| """ | ||
| if not project_id and not (project_name and project_version): | ||
| self.print_stderr('Error: Missing project id or name and version.') | ||
| return False | ||
| output = self.output | ||
| if output_file: | ||
| output = output_file | ||
| try: | ||
| self.print_debug('Encoding SBOM to base64') | ||
| payload = _build_payload(self._encode_sbom(sbom_content), project_id, project_name, project_version) | ||
| url = f'{self.url}/api/v1/bom' | ||
| headers = {'Content-Type': 'application/json', 'X-Api-Key': self.apikey} | ||
| if self.trace: | ||
| self.print_trace(f'URL: {url}') | ||
| self.print_trace(f'Headers: {headers}') | ||
| self.print_trace(f'Payload keys: {list(payload.keys())}') | ||
|
|
||
| self.print_stderr(f'URL: {url}') | ||
| self.print_stderr(f'Headers: {headers}') | ||
| self.print_stderr(f'Payload keys: {list(payload.keys())}') | ||
| self.print_msg('Uploading SBOM to Dependency Track...') | ||
| response = requests.put(url, json=payload, headers=headers) | ||
|
|
||
| if response.status_code in [200, 201]: | ||
| self.print_stderr('SBOM uploaded successfully') | ||
|
|
||
| response.raise_for_status() | ||
| if response.status_code in [requests.codes.ok, requests.codes.created]: | ||
| self.print_msg('SBOM uploaded successfully') | ||
| try: | ||
| response_data = response.json() | ||
| token = '' | ||
| project_uuid = project_id | ||
| if 'token' in response_data: | ||
| self.print_stderr(f'Upload token: {response_data["token"]}') | ||
| token = response_data['token'] | ||
| if project_name and project_version: | ||
| project_data = self.dt_service.get_project_by_name_version(project_name, project_version) | ||
| if project_data: | ||
| project_uuid = project_data.get("uuid", project_id) | ||
| token_json = json.dumps( | ||
| {"token": token, "project_uuid": project_uuid}, | ||
| indent=2 | ||
| ) | ||
| self.print_to_file_or_stdout(token_json, output) | ||
| except json.JSONDecodeError: | ||
| pass | ||
|
|
||
| return True | ||
| else: | ||
| self.print_stderr(f'Upload failed with status code: {response.status_code}') | ||
| self.print_stderr(f'Response: {response.text}') | ||
| return False | ||
|
|
||
| except ValueError as e: | ||
| self.print_stderr(f'Validation error: {e}') | ||
| return False | ||
| self.print_stderr(f'DT SBOM Upload Validation error: {e}') | ||
| except requests.exceptions.RequestException as e: | ||
| self.print_stderr(f'Request error: {e}') | ||
| return False | ||
| self.print_stderr(f'DT API Request error: {e}') | ||
| except Exception as e: | ||
| self.print_stderr(f'Unexpected error: {e}') | ||
| if self.debug: | ||
| traceback.print_exc() | ||
| return False | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce method complexity to address pipeline failure.
The method has 13 branches, exceeding the recommended limit of 12. The logic is sound but should be refactored for better maintainability.
Suggested refactoring approach:
Break the method into smaller focused methods:
def upload_sbom_contents(self, sbom_content: dict, project_id, project_name, project_version, output_file) -> bool:
if not self._validate_project_params(project_id, project_name, project_version):
return False
try:
response = self._send_sbom_request(sbom_content, project_id, project_name, project_version)
return self._process_upload_response(response, project_id, project_name, project_version, output_file)
except Exception as e:
self._handle_upload_error(e)
return False
def _validate_project_params(self, project_id, project_name, project_version) -> bool:
if not project_id and not (project_name and project_version):
self.print_stderr('Error: Missing project id or name and version.')
return False
return True
def _send_sbom_request(self, sbom_content, project_id, project_name, project_version):
# Handle encoding and HTTP request
def _process_upload_response(self, response, project_id, project_name, project_version, output_file) -> bool:
# Handle response processing and output generationThis would distribute the complexity across focused methods while maintaining the same functionality.
🧰 Tools
🪛 GitHub Actions: Lint
[error] 153-153: Ruff: Too many branches in function (13 > 12) (PLR0912)
🤖 Prompt for AI Agents
In src/scanoss/export/dependency_track.py from lines 153 to 223, the
upload_sbom_contents method exceeds the recommended complexity with 13 branches.
Refactor by splitting it into smaller methods: one to validate project
parameters (_validate_project_params), one to handle encoding and sending the
HTTP request (_send_sbom_request), and one to process the response and output
(_process_upload_response). Also add a method to handle errors
(_handle_upload_error). This will reduce complexity and improve maintainability
while preserving existing functionality.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/scanoss/export/dependency_track.py (1)
153-220: Method complexity remains high despite improvements.While the refactoring has improved error handling and parameter validation, the method still appears to exceed the complexity threshold with multiple branches handling validation, HTTP requests, response processing, and exception handling.
Consider the suggested approach from previous reviews: break this into focused methods like
_validate_project_params,_send_sbom_request, and_process_upload_response.
🧹 Nitpick comments (1)
src/scanoss/export/dependency_track.py (1)
198-200: Fix indentation inconsistency.Lines 198-200 use incorrect indentation (spaces instead of consistent indentation). This creates inconsistent formatting in the codebase.
Apply this diff to fix the indentation:
- if project_name and project_version: - project_data = self.dt_service.get_project_by_name_version(project_name, project_version) - if project_data: - project_uuid = project_data.get("uuid", project_id) + if project_name and project_version: + project_data = self.dt_service.get_project_by_name_version(project_name, project_version) + if project_data: + project_uuid = project_data.get("uuid", project_id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/scanoss/export/dependency_track.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/scanoss/export/dependency_track.py (5)
src/scanoss/cyclonedx.py (1)
CycloneDx(39-406)src/scanoss/scanossbase.py (4)
ScanossBase(28-107)print_trace(65-70)print_msg(51-56)print_to_file_or_stdout(83-94)src/scanoss/services/dependency_track_service.py (2)
DependencyTrackService(31-131)get_project_by_name_version(49-69)src/scanoss/utils/file.py (1)
validate_json_file(44-84)src/scanoss/cli.py (2)
print_stderr(86-90)export_dt(1491-1520)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/scanoss/export/dependency_track.py (3)
37-55: LGTM! Good separation of concerns.The extracted
_build_payloadfunction clearly handles the two different payload structures needed for the Dependency Track API, improving code maintainability and readability.
62-86: Constructor complexity successfully reduced.Good refactoring that addresses the previous complexity concern by reducing parameters from 6 to 5. The integration with
DependencyTrackServiceis well-structured and the URL normalization is handled appropriately.
126-151: Well-structured method with clear separation of concerns.The refactored method properly separates file validation from content uploading, with improved error handling and comprehensive documentation. The explicit parameter approach enhances clarity.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/scanoss/file_filters.py (2)
681-686: Align should_skip_dir with HFH vs. non-HFH listsCurrently this uses DEFAULT_SKIPPED_DIRS unconditionally; for folder hashing scans, it should consult DEFAULT_SKIPPED_DIRS_HFH to keep behavior consistent with _get_operation_patterns and debug messaging.
- dir_name_lower = dir_name.lower() - if dir_name_lower in DEFAULT_SKIPPED_DIRS: + dir_name_lower = dir_name.lower() + default_skipped_dirs = DEFAULT_SKIPPED_DIRS_HFH if self.is_folder_hashing_scan else DEFAULT_SKIPPED_DIRS + if dir_name_lower in default_skipped_dirs: self.print_debug(f'Skipping directory: {dir_rel_path} (matches default skip directory)') return True
523-529: Ensure rel_path is a string before pattern checks (avoid Path passed to pathspec)rel_path may be a Path when derived via relative_to(); later it’s passed into _should_skip_file -> match_file(), which expects a str. Cast to str to avoid subtle type issues.
- if scan_root: - rel_path = path_obj.relative_to(scan_root) + if scan_root: + rel_path = str(path_obj.relative_to(scan_root)) else: rel_path = str(path_obj)
🧹 Nitpick comments (3)
src/scanoss/file_filters.py (3)
686-688: Make user-provided skip_folders matching case-insensitiveOn case-insensitive filesystems, this avoids surprises and aligns with earlier normalization to lowercase for defaults.
- if self.skip_folders and dir_name in self.skip_folders: + if self.skip_folders and dir_name_lower in {s.lower() for s in self.skip_folders}: self.print_debug(f'Skipping directory: {dir_rel_path} (matches skip folder)') return True
497-499: Gate symlink directory warning behind debugThis currently prints on stdout during normal runs; prefer debug-level to reduce noise.
- self.print_msg(f'WARNING: Found symbolic link folder: {dir_path}') + self.print_debug(f'Found symbolic link folder: {dir_path}')
63-76: Confirm intentional inclusion of docs/ and test/ directories in scansSearch across the repository shows no existing tests or code that assume
docs/,doc/,test/ortests/were skipped by default. Removing these fromDEFAULT_SKIPPED_DIRSwill cause documentation and test folders to be scanned, which can increase output noise and impact performance.Please confirm that this broader default scope is intended. If backward compatibility is required or you’d rather keep skipping those folders by default, consider either:
- Re-introducing
'doc','docs','test', and'tests'intoDEFAULT_SKIPPED_DIRSinsrc/scanoss/file_filters.py(line 63).- Adding a CLI/configuration flag to toggle inclusion of documentation and test directories.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/scanoss/file_filters.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores