feat: implement exclude settings #136
Conversation
co-authored-by: Agustin Groh <[email protected]>
WalkthroughThe changes update the scan settings logic to support an "exclude" list of components in the Bill of Materials (BOM) for both legacy and new settings file types. New methods and logic branches are introduced to handle exclusion, and the scan type assignment is adjusted accordingly in both the CLI and settings handler. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ScanossSettings
User->>CLI: Run scan command (no --identify/--ignore)
CLI->>ScanossSettings: Load JSON settings file
alt New settings file
alt 'include' list present
ScanossSettings->>CLI: Set file_type='new', scan_type='identify', return include list
else 'exclude' list present
ScanossSettings->>CLI: Set file_type='new', scan_type='blacklist', return exclude list
else
ScanossSettings->>CLI: Set file_type='new', no scan_type
end
else Legacy settings file
alt scan_type='identify'
ScanossSettings->>CLI: Return combined include and replace lists
else scan_type='blacklist'
ScanossSettings->>CLI: Return exclude list
end
end
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/scanoss/cli.py(1 hunks)src/scanoss/scanoss_settings.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/scanoss/cli.py (1)
src/scanoss/scanoss_settings.py (2)
load_json_file(103-138)set_file_type(140-152)
src/scanoss/scanoss_settings.py (1)
src/scanoss/scanossbase.py (1)
print_debug(58-63)
⏰ 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 (4)
src/scanoss/scanoss_settings.py (3)
175-175: Good catch on the typo correction!The change from "Billing of Materials" to "Bill of Materials" is the correct terminology.
200-209: LGTM! The new method follows the established pattern.The
get_bom_exclude()method correctly mirrors the implementation pattern of existing methods likeget_bom_include()andget_bom_remove().
250-259: Review scan_type determination for new settings_file_typeThe automatic setting of
scan_typeis confined tosrc/scanoss/scanoss_settings.pyand no other parts of the codebase (including the CLI) or any tests referencescan_type. Before merging, please:
- Verify that the CLI logic which consumes the payload from
scanoss_settingscorrectly handles the new"identify"and"blacklist"values whensettings_file_type == 'new'.- Add or update tests to cover:
- When only
bom_includeis provided →scan_type == 'identify'- When only
bom_excludeis provided →scan_type == 'blacklist'- Behavior when both or neither lists are present
src/scanoss/cli.py (1)
1074-1075: Verify automatic scan type determinationThe removal of the explicit
.set_scan_type('identify')call at
src/scanoss/cli.py:1074–1075now delegates scan-type assignment to
ScanossSettings._get_sbom_assets(). That method only setsscan_typewhen either the include or exclude list is non-empty, so for a “new” settings file with neither list populated you’ll fall back to whatever default (or unset) state.Please add or confirm tests for these scenarios:
- “new” settings with only
bom.include→ expectscan_type = 'identify'- “new” settings with only
bom.exclude→ expectscan_type = 'blacklist'- “new” settings with neither list → verify default
scan_typeand asset payload (e.g. error, null assets, etc.)Ensure the observed behavior matches requirements.
| @@ -244,6 +266,14 @@ def _get_sbom_assets(self): | |||
| f'From Replace list: {[entry["purl"] for entry in replace_bom_entries]} \n' | |||
| ) | |||
| return include_bom_entries + replace_bom_entries | |||
|
|
|||
| if self.settings_file_type == 'legacy' and self.scan_type == 'blacklist': # sbom-identify.json | |||
There was a problem hiding this comment.
Fix incorrect comment.
The comment mentions "sbom-identify.json" but this code block handles the 'blacklist' scan type.
- if self.settings_file_type == 'legacy' and self.scan_type == 'blacklist': # sbom-identify.json
+ if self.settings_file_type == 'legacy' and self.scan_type == 'blacklist': # sbom-exclude.json📝 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.
| if self.settings_file_type == 'legacy' and self.scan_type == 'blacklist': # sbom-identify.json | |
| if self.settings_file_type == 'legacy' and self.scan_type == 'blacklist': # sbom-exclude.json |
🤖 Prompt for AI Agents
In src/scanoss/scanoss_settings.py at line 270, the comment incorrectly
references "sbom-identify.json" while the code handles the 'blacklist' scan
type. Update the comment to accurately describe the condition related to the
'blacklist' scan type instead of mentioning "sbom-identify.json".
|
|
||
| if self.settings_file_type == 'new': | ||
| if len(self.get_bom_include()): | ||
| self.scan_type = 'identify' | ||
| include_bom_entries = self._remove_duplicates(self.normalize_bom_entries(self.get_bom_include())) | ||
| return {"components": include_bom_entries} | ||
| elif len(self.get_bom_exclude()): | ||
| self.scan_type = 'blacklist' | ||
| exclude_bom_entries = self._remove_duplicates(self.normalize_bom_entries(self.get_bom_exclude())) | ||
| return {"components": exclude_bom_entries} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to avoid side effects in getter method.
The _get_sbom_assets() method is modifying self.scan_type as a side effect, which violates the single responsibility principle. Getter methods should retrieve data without modifying state.
Additionally, if both 'include' and 'exclude' lists are present, only the 'include' list is processed. Consider documenting this behavior or handling both lists appropriately.
Consider refactoring to separate concerns:
def _get_sbom_assets(self):
"""
Get the SBOM assets
Returns:
List: List of SBOM assets
"""
if self.settings_file_type == 'new':
if len(self.get_bom_include()):
- self.scan_type = 'identify'
include_bom_entries = self._remove_duplicates(self.normalize_bom_entries(self.get_bom_include()))
return {"components": include_bom_entries}
elif len(self.get_bom_exclude()):
- self.scan_type = 'blacklist'
exclude_bom_entries = self._remove_duplicates(self.normalize_bom_entries(self.get_bom_exclude()))
return {"components": exclude_bom_entries}Move the scan type determination to a separate method or handle it where _get_sbom_assets() is called.
📝 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.
| if self.settings_file_type == 'new': | |
| if len(self.get_bom_include()): | |
| self.scan_type = 'identify' | |
| include_bom_entries = self._remove_duplicates(self.normalize_bom_entries(self.get_bom_include())) | |
| return {"components": include_bom_entries} | |
| elif len(self.get_bom_exclude()): | |
| self.scan_type = 'blacklist' | |
| exclude_bom_entries = self._remove_duplicates(self.normalize_bom_entries(self.get_bom_exclude())) | |
| return {"components": exclude_bom_entries} | |
| def _get_sbom_assets(self): | |
| """ | |
| Get the SBOM assets | |
| Returns: | |
| List: List of SBOM assets | |
| """ | |
| if self.settings_file_type == 'new': | |
| if len(self.get_bom_include()): | |
| include_bom_entries = self._remove_duplicates( | |
| self.normalize_bom_entries(self.get_bom_include()) | |
| ) | |
| return {"components": include_bom_entries} | |
| elif len(self.get_bom_exclude()): | |
| exclude_bom_entries = self._remove_duplicates( | |
| self.normalize_bom_entries(self.get_bom_exclude()) | |
| ) | |
| return {"components": exclude_bom_entries} | |
| # … existing logic for legacy settings_file_type … |
🤖 Prompt for AI Agents
In src/scanoss/scanoss_settings.py around lines 249 to 259, the method
_get_sbom_assets() improperly modifies self.scan_type, causing side effects in a
getter. Refactor by removing the assignment to self.scan_type from this method
and instead determine and set scan_type in a separate method or at the call site
before invoking _get_sbom_assets(). Also, clarify or handle the case when both
include and exclude lists are present, either by documenting the current
behavior or adjusting logic to process both appropriately.
Summary by CodeRabbit