Skip to content
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.

Sign up for GitHub

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

Add Support GitHub Enterprise Server URL pattern #1270

Closed
wants to merge 2 commits into from
Closed

Add Support GitHub Enterprise Server URL pattern #1270

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c281eb1
refactor: PR and issue URL parsing for broader compatibility
yamao-latte Oct 5, 2024
cebfb5d
refactor: Improve parsing of PR and issue URLs for broader compatibility
yamao-latte Oct 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 26 additions & 34 deletions pr_agent/git_providers/github_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import itertools
import re
import time
import hashlib
from datetime import datetime
Expand Down Expand Up @@ -28,7 +29,14 @@ def __init__(self, pr_url: Optional[str] = None):
self.installation_id = None
self.max_comment_chars = 65000
self.base_url = get_settings().get("GITHUB.BASE_URL", "https://api.github.com").rstrip("/")
self.base_url_html = self.base_url.split("api/")[0].rstrip("/") if "api/" in self.base_url else "https://github.com"
if self.base_url.endswith("/api/v3"):
# enterprise server with api/v3
self.base_url_html = self.base_url[:len("/api/v3")]
elif self.base_url == "https://api.github.com":
# github cloud
self.base_url_html = "https://github.com"
else:
self.base_url_html = self.base_url
Comment on lines +32 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explain this change.
why is it needed ?

Give concrete examples - what didn't work before, and is working now due to this change ?

self.base_domain = self.base_url.replace("https://", "").replace("http://", "")
self.base_domain_html = self.base_url_html.replace("https://", "").replace("http://", "")
self.github_client = self._get_github_client()
Expand Down Expand Up @@ -611,51 +619,35 @@ def remove_reaction(self, issue_comment_id: int, reaction_id: str) -> bool:
def _parse_pr_url(self, pr_url: str) -> Tuple[str, int]:
parsed_url = urlparse(pr_url)

path_parts = parsed_url.path.strip('/').split('/')
if self.base_domain in parsed_url.netloc:
if len(path_parts) < 5 or path_parts[3] != 'pulls':
raise ValueError("The provided URL does not appear to be a GitHub PR URL")
repo_name = '/'.join(path_parts[1:3])
pattern = r'/repos/([^/]+/[^/]+)/pulls?/(\d+)'
Copy link
Collaborator

@mrT23 mrT23 Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this format support

https://github.com/Codium-ai/pr-agent/pull/1270

for example ?

Copy link
Author

@yamao-latte yamao-latte Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would like to support a format such as:
https://github.enterprise-server.com/api/v3/repos/Codium-ai/pr-agent/pull/1270

In GitHub Enterprise Server, the API URLs include the api/v3 segment. The current implementation uses a hard-coded check:

if len(path_parts) < 5 or path_parts[3] != 'pulls':

This approach does not account for the api/v3 part of the URL, leading to errors when the URL structure differs. Specifically, this issue affects the following part of the code:

For more information on the URL structure, refer to the GitHub Enterprise Server documentation:
GitHub Docs - Get a Pull Request

pr-agent/pr_agent/servers/github_action_runner.py

Lines 91 to 115 in ada0a3d

pr_url = event_payload.get("pull_request", {}).get("url")
if pr_url:
# legacy - supporting both GITHUB_ACTION and GITHUB_ACTION_CONFIG
auto_review = get_setting_or_env("GITHUB_ACTION.AUTO_REVIEW", None)
if auto_review is None:
auto_review = get_setting_or_env("GITHUB_ACTION_CONFIG.AUTO_REVIEW", None)
auto_describe = get_setting_or_env("GITHUB_ACTION.AUTO_DESCRIBE", None)
if auto_describe is None:
auto_describe = get_setting_or_env("GITHUB_ACTION_CONFIG.AUTO_DESCRIBE", None)
auto_improve = get_setting_or_env("GITHUB_ACTION.AUTO_IMPROVE", None)
if auto_improve is None:
auto_improve = get_setting_or_env("GITHUB_ACTION_CONFIG.AUTO_IMPROVE", None)
# Set the configuration for auto actions
get_settings().config.is_auto_command = True # Set the flag to indicate that the command is auto
get_settings().pr_description.final_update_message = False # No final update message when auto_describe is enabled
get_logger().info(f"Running auto actions: auto_describe={auto_describe}, auto_review={auto_review}, auto_improve={auto_improve}")
# invoke by default all three tools
if auto_describe is None or is_true(auto_describe):
await PRDescription(pr_url).run()
if auto_review is None or is_true(auto_review):
await PRReviewer(pr_url).run()
if auto_improve is None or is_true(auto_improve):
await PRCodeSuggestions(pr_url).run()

Copy link
Collaborator

@mrT23 mrT23 Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this PR won't provide complete and total support also for regular github, it cannot be merged.

All the issues I stated above should be addressed and fixed.

For example, currently
https://github.com/Codium-ai/pr-agent/pull/1266 (CLI)
is not supported in this new code . it was supported before

match = re.match(pattern, parsed_url.path)

if match:
repo_name = match.group(1)
try:
pr_number = int(path_parts[4])
pr_number = int(match.group(2))
except ValueError as e:
raise ValueError("Unable to convert PR number to integer") from e
return repo_name, pr_number

if len(path_parts) < 4 or path_parts[2] != 'pull':
else:
raise ValueError("The provided URL does not appear to be a GitHub PR URL")

repo_name = '/'.join(path_parts[:2])
try:
pr_number = int(path_parts[3])
except ValueError as e:
raise ValueError("Unable to convert PR number to integer") from e

return repo_name, pr_number

def _parse_issue_url(self, issue_url: str) -> Tuple[str, int]:
parsed_url = urlparse(issue_url)
path_parts = parsed_url.path.strip('/').split('/')
if self.base_domain in parsed_url.netloc:
if len(path_parts) < 5 or path_parts[3] != 'issues':
raise ValueError("The provided URL does not appear to be a GitHub ISSUE URL")
repo_name = '/'.join(path_parts[1:3])
path = parsed_url.path

pattern = r'/repos/([^/]+/[^/]+)/issues/(\d+)'
Copy link
Collaborator

@mrT23 mrT23 Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this format support:

https://github.com/Codium-ai/pr-agent/issues/1268

for example
?

match = re.match(pattern, path)

if match:
repo_name = match.group(1)
try:
issue_number = int(path_parts[4])
issue_number = int(match.group(2))
except ValueError as e:
raise ValueError("Unable to convert issue number to integer") from e
return repo_name, issue_number

if len(path_parts) < 4 or path_parts[2] != 'issues':
raise ValueError("The provided URL does not appear to be a GitHub PR issue")

repo_name = '/'.join(path_parts[:2])
try:
issue_number = int(path_parts[3])
except ValueError as e:
raise ValueError("Unable to convert issue number to integer") from e

return repo_name, issue_number
else:
raise ValueError("The provided URL does not appear to be a GitHub issue URL")

def _get_github_client(self):
deployment_type = get_settings().get("GITHUB.DEPLOYMENT_TYPE", "user")
Expand Down