gemini-code-assist[bot] commented on code in PR #39177:
URL: https://github.com/apache/beam/pull/39177#discussion_r3501433319
##########
infra/enforcement/sending.py:
##########
@@ -248,18 +248,36 @@ def create_announcement(self, title: str, body: str,
recipient: str, announcemen
"""
open_issues = self._get_open_issues(title)
open_issues.sort(key=lambda x: x.updated_at, reverse=True)
+
+ timestamp =
__import__("datetime").datetime.now(__import__("datetime").timezone.utc).strftime("%Y-%m-%d
%H:%M:%S UTC")
+ new_report = f"### Compliance Audit Report ({timestamp})\n{body}"
+
if open_issues:
- self.logger.info(f"Issue with title '{title}' already exists:
#{open_issues[0].number}")
- announcement += f"\n\nRelated GitHub Issue:
{open_issues[0].html_url}"
+ target_issue = open_issues[0]
+ self.logger.info(f"Issue with title '{title}' already exists:
#{target_issue.number}")
+ announcement += f"\n\nRelated GitHub Issue:
{target_issue.html_url}"
+
+ old_body = target_issue.body or ""
+ history_marker = "### History\n<details>\n<summary>Click to
expand</summary>\n\n"
- if open_issues[0].body != body:
- self.logger.info(f"Updating body of issue
#{open_issues[0].number}")
- self.update_issue_body(open_issues[0].number, body)
+ if history_marker in old_body:
+ # If history already exists, append the new report to it
+ headed = old_body.split(history_marker)
+ last_report = headed[0].strip()
+ old_history = headed[1].replace("</details>", "").strip()
+ combined_history = f"{last_report}\n\n---\n\n{old_history}"
Review Comment:
**Correctness/Robustness**: Using `.replace("</details>", "")` is risky
because it will remove *all* occurrences of `</details>` within the history,
which will corrupt the Markdown rendering if there are other collapsed sections
or nested HTML details blocks. Additionally, `split(history_marker)` should
limit the split to 1 to prevent issues if the marker appears elsewhere.
Instead, split with `maxsplit=1` and safely strip only the trailing
`</details>` tag.
```suggestion
headed = old_body.split(history_marker, 1)
last_report = headed[0].strip()
old_history = headed[1].rstrip()
if old_history.endswith("</details>"):
old_history = old_history[:-10].rstrip()
combined_history = f"{last_report}\n\n---\n\n{old_history}"
```
##########
infra/enforcement/sending.py:
##########
@@ -273,6 +291,13 @@ def print_announcement(self, title: str, body: str,
recipient: str, announcement
print(f"Recipient: {recipient}")
print(f"Announcement: {announcement}")
- print("\nSimulating GitHub issue creation...")
- print(f"Title: {title}")
- print(f"Body: {body}")
+ timestamp =
__import__("datetime").datetime.now(__import__("datetime").timezone.utc).strftime("%Y-%m-%d
%H:%M:%S UTC")
Review Comment:
**Style/Maintainability**: Using inline `__import__("datetime")` is
non-idiomatic and violates PEP 8 guidelines.
Please import `datetime` and `timezone` at the top of the file and simplify
this line.
```suggestion
timestamp = datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S
UTC")
```
<details>
<summary>References</summary>
1. Imports should be placed at the top of the file, as per PEP 8 guidelines.
<sup>([link](https://github.com/apache/beam/blob/master/.gemini/styleguide.md))</sup>
</details>
##########
infra/enforcement/account_keys.py:
##########
@@ -405,23 +405,44 @@ def print_announcement(self, recipient: str) -> None:
"""
if not self.sending_client:
raise ValueError("SendingClient is required for printing
announcements")
-
+
diff = self.check_compliance()
if not diff:
self.logger.info("No compliance issues found, no announcement will
be printed.")
return
- title = f"Account Keys Compliance Issue Detected"
- body = f"Account keys for project {self.project_id} are not compliant
with the defined policies on {self.service_account_keys_file}\n\n"
- for issue in diff:
- body += f"- {issue}\n"
+ unmanaged_keys_issues = [issue for issue in diff if "SECURITY ALERT"
in issue]
+ general_issues = [issue for issue in diff if "SECURITY ALERT" not in
issue]
+
+ if general_issues:
+ self.logger.info("Printing general compliance announcement...")
+ title = f"[SECURITY] Action Required: Unauthorized Service
Accounts Detected"
+ body = f"Unauthorized Service Accounts Report\n\n"
+ body += f"Account keys for project {self.project_id} are not
compliant with the defined policies on {self.service_account_keys_file}\n\n"
+ for issue in general_issues:
+ body += f"- {issue}\n"
+
+ announcement = f"Dear team,\n\nThis is an automated notification
about compliance issues detected in the Account Keys policy for project
{self.project_id}.\n\n"
+ announcement += f"We found {len(general_issues)} compliance
issue(s) that need your attention.\n"
+ announcement += f"\nPlease check the GitHub issue for detailed
information and take appropriate action to resolve these compliance violations."
- announcement = f"Dear team,\n\nThis is an automated notification about
compliance issues detected in the Account Keys policy for project
{self.project_id}.\n\n"
- announcement += f"We found {len(diff)} compliance issue(s) that need
your attention.\n"
- announcement += f"\nPlease check the GitHub issue for detailed
information and take appropriate action to resolve these compliance violations."
+ self.sending_client.print_announcement(title, body, recipient,
announcement)
- self.sending_client.print_announcement(title, body, recipient,
announcement)
+ if unmanaged_keys_issues:
+ self.logger.info("Printing security dashboard update for unmanaged
keys...")
+ timestamp =
datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%d %H:%M:%S UTC")
Review Comment:
**Potential Runtime Error**: Ensure that `datetime` is imported at the top
of the file. If it is not imported, this line will raise a `NameError` at
runtime.
If `datetime` is not imported, please add `from datetime import datetime,
timezone` at the top of the file and update this line to use them directly.
```suggestion
timestamp = datetime.now(timezone.utc).strftime("%Y-%m-%d
%H:%M:%S UTC")
```
##########
infra/enforcement/sending.py:
##########
@@ -248,18 +248,36 @@ def create_announcement(self, title: str, body: str,
recipient: str, announcemen
"""
open_issues = self._get_open_issues(title)
open_issues.sort(key=lambda x: x.updated_at, reverse=True)
+
+ timestamp =
__import__("datetime").datetime.now(__import__("datetime").timezone.utc).strftime("%Y-%m-%d
%H:%M:%S UTC")
Review Comment:
**Style/Maintainability**: Using inline `__import__("datetime")` is
non-idiomatic and violates PEP 8 guidelines regarding imports being at the top
of the file.
Please import `datetime` and `timezone` at the top of the file:
```python
from datetime import datetime, timezone
```
And simplify this line.
```suggestion
timestamp = datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M:%S
UTC")
```
<details>
<summary>References</summary>
1. Imports should be placed at the top of the file, as per PEP 8 guidelines.
<sup>([link](https://github.com/apache/beam/blob/master/.gemini/styleguide.md))</sup>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
