-
Notifications
You must be signed in to change notification settings - Fork 10
feat(ignore): adding event logger for ignored comments #178
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| __author__ = 'socket.dev' | ||
| __version__ = '2.2.82' | ||
| __version__ = '2.2.83' | ||
| USER_AGENT = f'SocketPythonCLI/{__version__}' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |
| import traceback | ||
| import shutil | ||
| import warnings | ||
| from datetime import datetime, timezone | ||
| from uuid import uuid4 | ||
|
|
||
| from dotenv import load_dotenv | ||
| from git import InvalidGitRepositoryError, NoSuchPathError | ||
|
|
@@ -478,6 +480,17 @@ def main_code(): | |
|
|
||
| # Handle SCM-specific flows | ||
| log.debug(f"Flow decision: scm={scm is not None}, force_diff_mode={force_diff_mode}, force_api_mode={force_api_mode}, enable_diff={config.enable_diff}") | ||
|
|
||
| def _is_unprocessed(c): | ||
| """Check if an ignore comment has not yet been marked with '+1' reaction. | ||
| For GitHub, reactions['+1'] is already in the comment response (no extra call). | ||
| For GitLab, has_thumbsup_reaction() makes a lazy API call per comment.""" | ||
| if getattr(c, "reactions", {}).get("+1"): | ||
| return False | ||
| if hasattr(scm, "has_thumbsup_reaction") and scm.has_thumbsup_reaction(c.id): | ||
| return False | ||
| return True | ||
|
|
||
| if scm is not None and scm.check_event_type() == "comment": | ||
| # FIXME: This entire flow should be a separate command called "filter_ignored_alerts_in_comments" | ||
| # It's not related to scanning or diff generation - it just: | ||
|
|
@@ -489,6 +502,44 @@ def main_code(): | |
|
|
||
| if not config.disable_ignore: | ||
| comments = scm.get_comments_for_pr() | ||
|
|
||
| # Emit telemetry for ignore comments before +1 reaction is added. | ||
| # The +1 reaction (added by remove_comment_alerts) serves as the "processed" marker. | ||
| if "ignore" in comments: | ||
| unprocessed = [c for c in comments["ignore"] if _is_unprocessed(c)] | ||
| if unprocessed: | ||
| try: | ||
| events = [] | ||
| for c in unprocessed: | ||
| single = {"ignore": [c]} | ||
| ignore_all, ignore_commands = Comments.get_ignore_options(single) | ||
| user = getattr(c, "user", None) or getattr(c, "author", None) or {} | ||
| now = datetime.now(timezone.utc).isoformat() | ||
| shared_fields = { | ||
| "event_kind": "user-action", | ||
| "client_action": "ignore", | ||
| "alert_action": "error", | ||
| "event_sender_created_at": now, | ||
| "vcs_provider": integration_type, | ||
| "owner": config.repo.split("/")[0] if "/" in config.repo else "", | ||
| "repo": config.repo, | ||
| "pr_number": pr_number, | ||
| "ignore_all": ignore_all, | ||
| "sender_name": user.get("login") or user.get("username", ""), | ||
| "sender_id": str(user.get("id", "")), | ||
| } | ||
| if ignore_commands: | ||
| for name, version in ignore_commands: | ||
| events.append({**shared_fields, "event_id": str(uuid4()), "artifact_input": f"{name}@{version}"}) | ||
| elif ignore_all: | ||
| events.append({**shared_fields, "event_id": str(uuid4())}) | ||
|
|
||
| if events: | ||
| log.debug(f"Ignore telemetry: {len(events)} events to send") | ||
| client.post_telemetry_events(org_slug, events) | ||
| except Exception as e: | ||
| log.warning(f"Failed to send ignore telemetry: {e}") | ||
|
|
||
| log.debug("Removing comment alerts") | ||
| scm.remove_comment_alerts(comments) | ||
| else: | ||
|
|
@@ -504,9 +555,76 @@ def main_code(): | |
| # FIXME: this overwrites diff.new_alerts, which was previously populated by Core.create_issue_alerts | ||
| if not config.disable_ignore: | ||
| log.debug("Removing comment alerts") | ||
| alerts_before = list(diff.new_alerts) | ||
| diff.new_alerts = Comments.remove_alerts(comments, diff.new_alerts) | ||
|
Member
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. Why does this use
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. My understanding of this is that there are two different flows. Push flow: A new push happened. The CLI is about to create a new security comment. It filters alerts before generating the comment so ignored packages never appear. one |
||
|
|
||
| ignored_alerts = [a for a in alerts_before if a not in diff.new_alerts] | ||
| # Emit telemetry per-comment so each event carries the comment author. | ||
| unprocessed_ignore = [ | ||
| c for c in comments.get("ignore", []) | ||
| if _is_unprocessed(c) | ||
| ] | ||
| if ignored_alerts and unprocessed_ignore: | ||
| try: | ||
| events = [] | ||
| now = datetime.now(timezone.utc).isoformat() | ||
| for c in unprocessed_ignore: | ||
| single = {"ignore": [c]} | ||
| c_ignore_all, c_ignore_commands = Comments.get_ignore_options(single) | ||
| user = getattr(c, "user", None) or getattr(c, "author", None) or {} | ||
| sender_name = user.get("login") or user.get("username", "") | ||
| sender_id = str(user.get("id", "")) | ||
|
|
||
| # Match this comment's targets to the actual ignored alerts | ||
| matched_alerts = [] | ||
| if c_ignore_all: | ||
| matched_alerts = ignored_alerts | ||
| else: | ||
| for alert in ignored_alerts: | ||
| full_name = f"{alert.pkg_type}/{alert.pkg_name}" | ||
| purl = (full_name, alert.pkg_version) | ||
| purl_star = (full_name, "*") | ||
| if purl in c_ignore_commands or purl_star in c_ignore_commands: | ||
| matched_alerts.append(alert) | ||
|
|
||
| shared_fields = { | ||
| "event_kind": "user-action", | ||
| "client_action": "ignore", | ||
| "event_sender_created_at": now, | ||
| "vcs_provider": integration_type, | ||
| "owner": config.repo.split("/")[0] if "/" in config.repo else "", | ||
| "repo": config.repo, | ||
| "pr_number": pr_number, | ||
| "ignore_all": c_ignore_all, | ||
| "sender_name": sender_name, | ||
| "sender_id": sender_id, | ||
| } | ||
| if matched_alerts: | ||
| for alert in matched_alerts: | ||
| # Derive alert_action from the alert's resolved action flags | ||
| if getattr(alert, "error", False): | ||
| alert_action = "error" | ||
| elif getattr(alert, "warn", False): | ||
| alert_action = "warn" | ||
| elif getattr(alert, "monitor", False): | ||
| alert_action = "monitor" | ||
| else: | ||
| alert_action = "error" | ||
| events.append({**shared_fields, "alert_action": alert_action, "event_id": str(uuid4()), "artifact_purl": alert.purl}) | ||
| elif c_ignore_all: | ||
| events.append({**shared_fields, "event_id": str(uuid4())}) | ||
|
|
||
| if events: | ||
| client.post_telemetry_events(org_slug, events) | ||
|
|
||
| # Mark ignore comments as processed with +1 reaction | ||
| if hasattr(scm, "handle_ignore_reactions"): | ||
| scm.handle_ignore_reactions(comments) | ||
| except Exception as e: | ||
| log.warning(f"Failed to send ignore telemetry: {e}") | ||
| else: | ||
| log.info("Ignore commands disabled (--disable-ignore), all alerts will be reported") | ||
|
|
||
| log.debug("Creating Dependency Overview Comment") | ||
|
|
||
| overview_comment = Messages.dependency_overview_template(diff) | ||
|
|
||
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.
Is this the best way of marking comments processed? Seems a little easy for other human users to mess up. It's fine as a best-effort approach though
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.
This is mainly used to dedup the number of events logged.
first
ignore foomessage will cause the workflow to rerun with the cli, and it will log foo is being ignoredsecond
ignore barmessage will cause foo and bar to be logged, the +1 emoji attempts to deduplicate these logs, however, at the same time, it also means the user could immediately add the +1 emoji