Enhancement: Apply Repository Settings on Every 'Synchronize' Event

[mrT23]

Type

Enhancement

---

Description

This PR modifies the handling of the 'pull_request' event with 'synchronize' action in the 'github_app.py' file. The main changes include:

• The application of repository settings is now triggered on every 'synchronize' event, not only when the 'handle_push_trigger' setting is enabled.
• The 'handle_push_trigger' setting is now checked after applying the repository settings, and if it's not enabled, the function returns without further processing.

---

PR changes walkthrough

	Relevant files
Enhancement	1 files github_app.py                                                                                              pr_agent/servers/github_app.py The 'handle_request' function has been updated to apply repository settings on every 'synchronize' event. The check for the 'handle_push_trigger' setting is now performed after applying the repository settings. +5/-1

[github-actions]

PR Analysis

(review updated until commit a043eb9)

• 🎯 Main theme: Modifying the handling of the 'pull_request' event with 'synchronize' action in the 'github_app.py' file.
• 📝 PR summary: This PR changes the handling of the 'pull_request' event with 'synchronize' action. Now, the repository settings are applied on every 'synchronize' event, not only when the 'handle_push_trigger' setting is enabled. The 'handle_push_trigger' setting is checked after applying the repository settings, and if it's not enabled, the function returns without further processing.
• 📌 Type of PR: Enhancement
• 🧪 Relevant tests added: No
• ⏱️ Estimated effort to review [1-5]: 2, because the PR is small and the changes are straightforward, but it requires understanding of the existing codebase and the 'pull_request' event handling.
• 🔒 Security concerns: No security concerns found

PR Feedback

• 💡 General suggestions: It would be beneficial to add tests that verify the new behavior. This would ensure that the repository settings are indeed applied on every 'synchronize' event and that the function returns correctly when 'handle_push_trigger' is not enabled.

• 🤖 Code feedback:

  ---

  relevant file	pr_agent/servers/github_app.py
  suggestion	Consider handling the case where the 'apply_repo_settings' function fails. This could be done by wrapping the function call in a try-except block and logging any exceptions that occur. [important]
  relevant line	apply_repo_settings(api_url)

  ---

[github-actions]

PR Code Suggestions

💡 Suggestion:

Consider checking the return value of apply_repo_settings(api_url) before proceeding.

File: pr_agent/servers/github_app.py (133-135)

Example code:

---

Existing code:

apply_repo_settings(api_url)
if not get_settings().github_app.handle_push_trigger:
    return {}

Improved code:

if not apply_repo_settings(api_url) or not get_settings().github_app.handle_push_trigger:
    return {}

---

💡 Suggestion:

Consider handling the case where before_sha or after_sha is None.

File: pr_agent/servers/github_app.py (138-139)

Example code:

---

Existing code:

before_sha = body.get("before")
after_sha = body.get("after")

Improved code:

before_sha = body.get("before")
after_sha = body.get("after")
if before_sha is None or after_sha is None:
    log_context.error("Missing 'before' or 'after' in the request body.")
    return {}

---

[mrT23]

@zmeir @okotek
please take a look.

The issue raised in:
#504
is valid - on 'synchronize', we do not load the local configuration.
Hence, we will not trigger PR-Agent (unless the GithubApp itself was built with handle_push_trigger=true)

It's not ideal to read the configuration on every push. But currently the feature, and our instructions on the README, are not valid.
Can you think of a better way to enable this feature from local configuration file ?

[mrT23]

/describe

[github-actions]

PR Description updated to latest commit (a043eb9)

[mrT23]

PR Description updated to latest commit (a043eb9)

[zmeir]

LGTM. I can't think of a cleaner way to handle this. The only other option I can think of is to just explicitly state in the README that this feature can only be enabled/disables via the global settings. But then what's the point of having it anyway?

[mrT23]

Persistent review updated to latest commit a043eb9

[mrT23]

Persistent review updated to latest commit a043eb9