Repository navigation
LCORE-5177: Ability to select CVEs with one state #3120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,11 @@ | |
| repositories. | ||
|
|
||
| # Usage: | ||
| usage: vulnerability_report.py [-h] [-v] --organization ORGANIZATION | ||
| --repository REPOSITORY [-r] [-g] [-p] | ||
| [-c COMPARISON [COMPARISON ...]] | ||
| usage: vulnerability_report.py [-h] [-v] [--organization ORGANIZATION] | ||
| [--repository REPOSITORY] [-r] | ||
| [-s {auto_dismissed,dismissed,fixed,open}] [-a] | ||
| [-g] [-svg] [-png] [-p] | ||
| [-c COMPARISON [COMPARISON ...]] | ||
|
|
||
| Vulnerability report tool | ||
|
|
||
|
|
@@ -22,6 +24,8 @@ | |
| GitHub repository. | ||
| -r, --retrieve-issues | ||
| Retrieve issues | ||
| -s, --state {auto_dismissed,dismissed,fixed,open} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] incorrect-doc The inline usage docstring lists the -s/--state option twice on consecutive lines. Only one entry should appear. Suggested fix: Remove the duplicate line. Keep the {auto_dismissed,dismissed,fixed,open} variant with the help text. |
||
| Filter just issues with specified state during retrieval | ||
| -a, --all-actions Retrieve issues for all LCore repositories, generates graphs and reports | ||
| -g, --generate-graphs | ||
| Generate graphs with vulnerabilities info | ||
|
|
@@ -41,7 +45,7 @@ | |
| from collections import Counter | ||
| from datetime import datetime | ||
| from statistics import median | ||
| from typing import Any | ||
| from typing import Any, Optional | ||
|
|
||
| import matplotlib.pyplot as plt | ||
| import pandas as pd | ||
|
|
@@ -103,6 +107,14 @@ def create_argument_parser() -> ArgumentParser: | |
| help="Retrieve issues", | ||
| ) | ||
|
|
||
| cli_parser.add_argument( | ||
| "-s", | ||
| "--state", | ||
| default=None, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case The --state argument is silently ignored when --retrieve-issues (-r) is not specified in single-repo mode. If a user passes -s open but forgets -r, no retrieval happens and the filter has no effect with no warning. Suggested fix: Emit a warning when --state is supplied without --retrieve-issues. |
||
| choices=["auto_dismissed", "dismissed", "fixed", "open"], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| help="Filter just issues with specified state during retrieval", | ||
| ) | ||
|
|
||
| cli_parser.add_argument( | ||
| "-a", | ||
| "--all-actions", | ||
|
|
@@ -745,7 +757,12 @@ def is_valid_github_org_repo(name: str) -> bool: | |
| return bool(re.fullmatch(r"[A-Za-z0-9_.-]+", name)) | ||
|
|
||
|
|
||
| def retrieve_issues(organization: str, repository: str, dependabot_file: str) -> int: | ||
| def retrieve_issues( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] pattern-inconsistency In retrieve_issues() the parameter order is (organization, repository, dependabot_file, state_filter), but in process_issues_for_repository() the order is (organization, repository, state_filter, dependabot_file, ...). The parameter ordering is inconsistent but all call sites pass arguments correctly. Suggested fix: Align the parameter order between the two functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] missing-test The new state_filter parameter and the pagination request_params clearing logic have no automated test coverage. Suggested fix: Add unit tests for the state_filter parameter flow and the pagination request_params clearing behavior. |
||
| organization: str, | ||
| repository: str, | ||
| dependabot_file: str, | ||
| state_filter: Optional[str], | ||
| ) -> int: | ||
| """ | ||
| Retrieve all Dependabot alerts for a GitHub repository and save them to a JSON file. | ||
|
|
||
|
|
@@ -780,9 +797,9 @@ def retrieve_issues(organization: str, repository: str, dependabot_file: str) -> | |
| print("Repository name is not correct") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] scope-creep The rename of the local variable TOKEN to GITHUB_TOKEN is an opportunistic cleanup unrelated to the stated intent of LCORE-5177. |
||
| return 2 | ||
|
|
||
| TOKEN = os.environ.get("GITHUB_TOKEN") | ||
| GITHUB_TOKEN = os.environ.get("GITHUB_TOKEN") | ||
|
|
||
| if TOKEN is None: | ||
| if GITHUB_TOKEN is None: | ||
| print("GITHUB_TOKEN environment variable must be set!") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] naming-convention GITHUB_TOKEN uses UPPER_CASE for a local variable, inconsistent with other local variables in the same function. |
||
| return 1 | ||
|
|
||
|
|
@@ -793,8 +810,13 @@ def retrieve_issues(organization: str, repository: str, dependabot_file: str) -> | |
|
|
||
| url = f"https://api.github.com/repos/{organization}/{repository}/dependabot/alerts" | ||
|
|
||
| request_params = {} | ||
|
|
||
| if state_filter is not None: | ||
| request_params["state"] = state_filter | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle empty filtered results in graph generation. If a repository has no alerts in the selected state, the filtered result is empty. With
|
||
|
|
||
| headers = { | ||
| "Authorization": f"Bearer {TOKEN}", | ||
| "Authorization": f"Bearer {GITHUB_TOKEN}", | ||
| "Accept": "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| } | ||
|
|
@@ -806,7 +828,18 @@ def retrieve_issues(organization: str, repository: str, dependabot_file: str) -> | |
| try: | ||
| while url: | ||
| print("Retrieving data from URL:", url) | ||
| r = requests.get(url, headers=headers, timeout=10) | ||
| r = requests.get( | ||
| url, headers=headers, timeout=10, params=request_params | ||
| ) | ||
|
|
||
| # params with state filter need to be passed just for the 1st call | ||
| # when paginating through Dependabot alerts, request_params | ||
| # (containing the state filter) is re-sent on every iteration | ||
| # of the while url loop. GitHub pagination Link headers return | ||
| # fully-qualified next-page URLs that already include query | ||
| # parameters. | ||
| request_params = {} | ||
|
|
||
| r.raise_for_status() | ||
| data = r.json() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] edge-case When paginating through Dependabot alerts, request_params (containing the state filter) is re-sent on every iteration of the while url loop. GitHub pagination Link headers return fully-qualified next-page URLs that already include query parameters. Passing params=request_params on subsequent requests appends a duplicate state parameter. |
||
|
|
||
|
|
@@ -839,6 +872,7 @@ def retrieve_issues(organization: str, repository: str, dependabot_file: str) -> | |
| def process_issues_for_repository( | ||
| organization: str, | ||
| repository: str, | ||
| state_filter: Optional[str], | ||
| dependabot_file: str, | ||
| svg_output: bool, | ||
| png_output: bool, | ||
|
|
@@ -851,6 +885,7 @@ def process_issues_for_repository( | |
| Parameters: | ||
| organization (str): name of GitHub organization | ||
| repository (str): name of GitHub repository | ||
| state_filter (str): used to filter issues with specified state | ||
| dependabot_file (str): file to store Dependabot issues | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] pattern-inconsistency The process_issues_for_repository docstring documents state_filter as (str) but the parameter type is Optional[str]. Other parameters in this same docstring use the exact type from their signature. Suggested fix: Change state_filter (str) to state_filter (Optional[str]) in the Parameters block. |
||
| svg_output (bool): if set to true, CVE statistic is exported into SVG graphs | ||
| png_output (bool): if set to true, CVE statistic is exported into PNG graphs | ||
|
|
@@ -865,7 +900,9 @@ def process_issues_for_repository( | |
| result = 0 | ||
| if retrieve_issues_operation: | ||
| # retrieve issues | ||
| result += retrieve_issues(organization, repository, dependabot_file) | ||
| result += retrieve_issues( | ||
| organization, repository, dependabot_file, state_filter | ||
| ) | ||
|
|
||
| if result > 0: | ||
| print("Error retrieving issues") | ||
|
|
@@ -923,6 +960,7 @@ def main() -> int: | |
| errors += process_issues_for_repository( | ||
| organization, | ||
| repository, | ||
| args.state, | ||
| dependabot_file, | ||
| args.svg_output, | ||
| args.png_output, | ||
|
|
@@ -935,6 +973,7 @@ def main() -> int: | |
| return process_issues_for_repository( | ||
| args.organization, | ||
| args.repository, | ||
| args.state, | ||
| dependabot_file, | ||
| args.svg_output, | ||
| args.png_output, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] scope-creep
The diff includes updates to the usage docstring block beyond the --state flag (e.g., --organization becoming optional, addition of -a, -svg, -png flags), which appear to be corrections for pre-existing flags not previously documented in the usage block.