Repository navigation
LCORE-5177: Vulnerability script refactoring, enhancement, and fixes #3098
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
57b9ba4
4cb61fe
9defbf3
5e7af4a
925ea6c
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 |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| GitHub repository. | ||
| -r, --retrieve-issues | ||
| Retrieve issues | ||
| -a, --all-actions Retrieve issues for all LCore repositories, generates graphs and reports | ||
| -g, --generate-graphs | ||
| Generate graphs with vulnerabilities info | ||
| -svg, --svg-output Generate graphs in SVG format | ||
|
|
@@ -43,12 +44,21 @@ | |
| from typing import Any | ||
|
|
||
| import matplotlib.pyplot as plt | ||
| import pandas as pd | ||
|
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. [high] scope-creep The PR adds import pandas as pd but pandas is commented out in pyproject.toml (line 177). The script will raise ImportError at module load time in the project virtualenv. Suggested fix: Either uncomment and formally adopt pandas>=2.2.3 in pyproject.toml (following the Konflux dependency update process in AGENTS.md), or rewrite the moving-average/mean calculation using only the standard library or matplotlib. 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] scope-creep PR introduces a new pandas dependency solely for a 7-day moving average on the CVE timeline graph. Material scope expansion for a utility script. PR title includes enhancement which partially covers this. Suggested fix: Either split the pandas-based graph enhancement into its own PR, or implement with plain Python to avoid the new dependency. |
||
| import requests | ||
| from dateutil import parser | ||
|
|
||
| type DependabotAlert = dict[str, Any] | ||
|
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] design-direction LCORE_REPOSITORIES and DEFAULT_ORGANIZATION are hardcoded constants, coupling the script to a specific set of repositories. Suggested fix: Consider accepting a --repositories flag or a config file. |
||
| type DependabotAlerts = list[DependabotAlert] | ||
|
|
||
| DEFAULT_ORGANIZATION = "lightspeed-core" | ||
|
|
||
| LCORE_REPOSITORIES = [ | ||
| "lightspeed-stack", | ||
| "lightspeed-providers", | ||
| "rag-content", | ||
| ] | ||
|
|
||
|
|
||
| def create_argument_parser() -> ArgumentParser: | ||
| """Create and configure argument parser object. | ||
|
|
@@ -75,14 +85,14 @@ def create_argument_parser() -> ArgumentParser: | |
| "--organization", | ||
| default="", | ||
| help="GitHub organization.", | ||
| required=True, | ||
| required=False, | ||
| ) | ||
|
|
||
| cli_parser.add_argument( | ||
| "--repository", | ||
| default="", | ||
| help="GitHub repository.", | ||
| required=True, | ||
| required=False, | ||
| ) | ||
|
|
||
| cli_parser.add_argument( | ||
|
|
@@ -93,6 +103,14 @@ def create_argument_parser() -> ArgumentParser: | |
| help="Retrieve issues", | ||
| ) | ||
|
|
||
| cli_parser.add_argument( | ||
| "-a", | ||
| "--all-actions", | ||
| action="store_true", | ||
| default=False, | ||
| help="Retrieve issues for all LCore repositories, generates graphs and reports", | ||
| ) | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
|
|
||
| cli_parser.add_argument( | ||
| "-g", | ||
| "--generate-graphs", | ||
|
|
@@ -150,11 +168,18 @@ def check_args(args: Namespace) -> None: | |
| ValueError: If graph generation is requested but neither SVG nor PNG output is selected. | ||
|
|
||
| """ | ||
| if args.generate_graphs: | ||
| if args.all_actions or args.generate_graphs: | ||
| # when graphs should be generated, output format need to be specified too | ||
| if not args.svg_output and not args.png_output: | ||
| msg = "Graphs generation was requested: you need to select the PNG and/or SVG output." | ||
|
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] error-handling-idiom check_args raises ValueError for missing args, while other scripts rely on argparse required=True mechanism. Suggested fix: Use argparse mutually-exclusive groups or cli_parser.error() instead of raise ValueError. 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] error-handling-idiom The new validation error messages for missing --organization/--repository lack a trailing period, unlike the existing message on line 176 which ends with a period. Suggested fix: Add trailing periods to both error messages. |
||
| raise ValueError(msg) | ||
| if not args.all_actions: | ||
| if args.organization == "": | ||
| msg = "Organization name needs to be specified" | ||
|
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] error-handling-idiom check_args raises ValueError for missing organization/repository but the prior pattern used argparse required=True with clearer error messages. |
||
| raise ValueError(msg) | ||
| if args.repository == "": | ||
| msg = "Repository name needs to be specified" | ||
| raise ValueError(msg) | ||
|
|
||
|
|
||
| def dependabot_file_name(args: Namespace) -> str: | ||
|
|
@@ -540,7 +565,7 @@ def generate_vuln_for_days_graph( | |
| _, ax = plt.subplots() | ||
| data = stat["days"]["days"] | ||
| ax.hist(data, bins=30, edgecolor="black") | ||
| ax.set_ylim(top=100) | ||
| ax.set_ylim(top=150) | ||
| ax.set_xlabel("Days") | ||
| ax.set_title("Fixed in day(s)") | ||
| save_graph(prefix, "days", svg_output, png_output) | ||
|
|
@@ -585,11 +610,39 @@ def generate_new_cve_dates_graph( | |
| png_output (bool): Whether to save the graph as PNG. | ||
|
|
||
| """ | ||
| _, ax = plt.subplots() | ||
| _, ax = plt.subplots(figsize=(14, 7)) | ||
|
|
||
| data = stat["dates"] | ||
| if not data: | ||
| save_graph(prefix, "timeline", svg_output, png_output) | ||
| return | ||
|
|
||
| dates, counts = zip(*sorted(data.items(), key=lambda x: x[0])) | ||
|
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 zip(*sorted(data.items())) raises ValueError on an empty Counter. The new pandas operations amplify this pre-existing issue. 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 If stat dates is an empty Counter, zip(*sorted(data.items())) yields an empty iterator and tuple unpacking raises ValueError. More likely with --all-actions iterating over multiple repositories. Suggested fix: Add an early return guard: if not data: save_graph(...); return. |
||
| ax.plot(dates, counts) | ||
| ax.plot(dates, counts, label="CVEs per day") | ||
| ax.set_title("New CVEs timeline") | ||
| cve_series = pd.Series(counts, index=pd.to_datetime(list(dates))) | ||
| moving_average = cve_series.rolling(window=7, min_periods=1, center=True).mean() | ||
| ax.plot( | ||
| moving_average.index, | ||
| moving_average.values, | ||
| label="7-day moving average", | ||
| linewidth=2, | ||
| ) | ||
|
|
||
| # horizontal line with "total" average | ||
| avg = cve_series.mean() | ||
| ax.axhline( | ||
| avg, | ||
| color="red", | ||
| linestyle="--", | ||
| linewidth=1.5, | ||
| label=f"Day average ({avg:.2f})", | ||
| ) | ||
|
|
||
| ax.legend() | ||
|
|
||
| plt.setp(ax.get_xticklabels(), rotation=45, ha="right") | ||
| plt.subplots_adjust(bottom=0.20) | ||
| save_graph(prefix, "timeline", svg_output, png_output) | ||
|
|
||
|
|
||
|
|
@@ -733,6 +786,11 @@ def retrieve_issues(organization: str, repository: str, dependabot_file: str) -> | |
| print("GITHUB_TOKEN environment variable must be set!") | ||
| return 1 | ||
|
|
||
| print("Retrieving issues:") | ||
| print(f" organization: {organization}") | ||
| print(f" repository: {repository}") | ||
| print(f" output file: {dependabot_file}") | ||
|
|
||
| url = f"https://api.github.com/repos/{organization}/{repository}/dependabot/alerts" | ||
|
|
||
|
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 Prior edge-case finding in the retrieve_issues function area; code in this region is substantively unchanged. |
||
| headers = { | ||
|
|
@@ -772,12 +830,59 @@ def retrieve_issues(organization: str, repository: str, dependabot_file: str) -> | |
| f.write("]\n") | ||
| raise | ||
|
|
||
| print("Saved all alerts to dependabot_alerts.json") | ||
| print(f"Saved all alerts to {dependabot_file}") | ||
|
|
||
| # everything's ok | ||
| return 0 | ||
|
|
||
|
|
||
| def process_issues_for_repository( | ||
| organization: str, | ||
| repository: str, | ||
| dependabot_file: str, | ||
| svg_output: bool, | ||
| png_output: bool, | ||
| retrieve_issues_operation: bool = True, | ||
| generate_graphs_operation: bool = True, | ||
| generate_page_operation: bool = True, | ||
| ) -> int: | ||
| """Retrieve alerts or process the local file and generate selected outputs. | ||
|
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] doc-style process_issues_for_repository docstring omits the Parameters section unlike every other function with arguments in this file. |
||
|
|
||
| Parameters: | ||
|
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] doc-style The new process_issues_for_repository docstring is missing a Returns: section that other functions in this file include. Suggested fix: Add a Returns: section documenting the int exit code semantics. |
||
| organization (str): name of GitHub organization | ||
| repository (str): name of GitHub repository | ||
| dependabot_file (str): file to store Dependabot issues | ||
| 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 | ||
| retrieve_issues_operation (bool): enable retrieving Dependabot issues | ||
| generate_graphs_operation (bool): enable generating graphs in SVG or PNG format | ||
| generate_page_operation (bool): enable generating page with overall status | ||
|
|
||
| Retrieval takes precedence over report generation and returns its status. | ||
| Otherwise return 0 after processing; file, data, and plotting errors propagate. | ||
| """ | ||
| prefix = repository | ||
| result = 0 | ||
| if retrieve_issues_operation: | ||
| # retrieve issues | ||
| result += retrieve_issues(organization, repository, dependabot_file) | ||
|
|
||
| if result > 0: | ||
| print("Error retrieving issues") | ||
| print("Skipping graphs and pages generation") | ||
| return 1 | ||
|
|
||
| stat = process_dependabot_file(dependabot_file) | ||
|
|
||
|
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] logic-error Unconditional process_dependabot_file call is a behaviour change from the old code. Previously, --retrieve-issues caused an early return. In the refactored process_issues_for_repository, after a successful retrieval the function always falls through to stat = process_dependabot_file(dependabot_file) even when generate_graphs_operation and generate_page_operation are both False. Suggested fix: Guard process_dependabot_file behind a check: only call it when generate_graphs_operation or generate_page_operation is True. |
||
| if generate_graphs_operation: | ||
| generate_graphs(stat, prefix, svg_output, png_output) | ||
|
|
||
| if generate_page_operation: | ||
| generate_page(stat, prefix, organization, repository) | ||
|
|
||
| return 0 | ||
|
|
||
|
|
||
| def main() -> int: | ||
| """ | ||
| CLI entry point that retrieves Dependabot issues and produces Vulnerability report. | ||
|
|
@@ -809,22 +914,34 @@ def main() -> int: | |
| cli_parser = create_argument_parser() | ||
| args = cli_parser.parse_args() | ||
| check_args(args) | ||
|
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 When --all-actions is used, check_args validation for graph output format (--svg-output or --png-output) is bypassed. The --all-actions path passes True for graph generation without checking that an output format is specified, so save_graph silently produces no output. Suggested fix: Either validate output format flags when --all-actions is set, or default to a specific output format for the all-actions path.
Contributor
Author
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. /fs-fix |
||
| dependabot_file = dependabot_file_name(args) | ||
| prefix = args.repository | ||
|
|
||
| if args.retrieve_issues: | ||
| # just retrieve issues, no other operation will be performed | ||
| return retrieve_issues(args.organization, args.repository, dependabot_file) | ||
|
|
||
| stat = process_dependabot_file(dependabot_file) | ||
|
|
||
| if args.generate_graphs: | ||
| generate_graphs(stat, prefix, args.svg_output, args.png_output) | ||
|
|
||
|
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] logic-error In main(), the all_actions branch loops over repositories and calls process_issues_for_repository() but discards the return value. No return statement follows the loop, so main() implicitly returns None, which SystemExit(None) treats as exit code 0 — masking failures. The else branch correctly returns the int, and the -> int annotation is also violated. Suggested fix: Accumulate or check the return value of process_issues_for_repository inside the loop and return an appropriate int at the end of the if args.all_actions block. |
||
| if args.generate_page: | ||
| generate_page(stat, prefix, args.organization, args.repository) | ||
| if args.all_actions: | ||
| errors = 0 | ||
| for repository in LCORE_REPOSITORIES: | ||
| organization = DEFAULT_ORGANIZATION | ||
| dependabot_file = f"{organization}__{repository}.json" | ||
| errors += process_issues_for_repository( | ||
| organization, | ||
|
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] logic-error When --all-actions processes multiple repositories and more than one fails, errors accumulates return values so main() can return 2 or 3, violating the documented exit code contract (0 success, 1 failure). Suggested fix: Clamp the return: return min(errors, 1) or return 1 if errors else 0. |
||
| repository, | ||
|
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] pattern-inconsistency In the all_actions branch of main(), the variable organization is reassigned to DEFAULT_ORGANIZATION inside the loop body on every iteration, though it never changes. Suggested fix: Move organization = DEFAULT_ORGANIZATION above the for loop. |
||
| dependabot_file, | ||
| args.svg_output, | ||
| args.png_output, | ||
| ) | ||
|
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] pattern-inconsistency The else branch after if args.all_actions return errors is unnecessary since the if block returns. |
||
| return 1 if errors > 0 else 0 | ||
|
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] pattern-inconsistency The dependabot_file name in the all_actions branch is built inline as f"{organization}__{repository}.json", duplicating the logic already in dependabot_file_name(). The single-repository branch correctly delegates to that helper. Suggested fix: Refactor dependabot_file_name() to accept organization and repository strings directly, then call it from both branches. |
||
|
|
||
| # processing for one selected repository | ||
| dependabot_file = dependabot_file_name(args) | ||
|
|
||
| return 0 | ||
| return process_issues_for_repository( | ||
| args.organization, | ||
| args.repository, | ||
| dependabot_file, | ||
| args.svg_output, | ||
| args.png_output, | ||
| args.retrieve_issues, | ||
| args.generate_graphs, | ||
| args.generate_page, | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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] over-engineering
The pandas library (~30 MB installed) is added as a dev dependency solely to compute a 7-day rolling average and a mean on a small integer series. numpy (already a transitive dependency of matplotlib) provides np.convolve for a rolling average and np.mean, which would accomplish the same thing without pulling in an additional heavy dependency.
Suggested fix: Replace pd.Series/.rolling/.mean with numpy equivalents (np.convolve for rolling average, np.mean for the overall average).