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.

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

Bitbucket Server fix inline comments and reading .pr_agent.toml #1081

Conversation

MarkRx
Copy link
Contributor

@MarkRx MarkRx commented Jul 30, 2024

User description

  • Fixes BB inline comments. The current implementation fails with a 401 unauthorized the first time the service is executed since the requests context won't have the BB bearer token. Switched to using the BB client
  • Fixes BB loading of .pr_agent.toml file. The current implementation doesn't work since it uses the wrong field to obtain branch information
  • Removed raw REST calls in favor of using BB client

PR Type

Bug fix, Enhancement


Description

  • Replaced raw REST API calls with Bitbucket client methods to improve reliability and maintainability.
  • Fixed an issue causing 401 unauthorized errors when posting inline comments by ensuring the correct bearer token is used.
  • Corrected the loading of the .pr_agent.toml file by using the correct branch information.
  • Added improved error handling for HTTP errors, specifically handling 404 not found errors when loading the .pr_agent.toml file.

Changes walkthrough 📝

Relevant files
Enhancement
bitbucket_server_provider.py
Refactor Bitbucket Server provider to use Bitbucket client

pr_agent/git_providers/bitbucket_server_provider.py

  • Replaced raw REST calls with Bitbucket client methods.
  • Fixed 401 unauthorized error when posting inline comments.
  • Corrected loading of .pr_agent.toml file using the correct branch
    information.
  • Improved error handling for HTTP errors.
  • +14/-24 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added enhancement New feature or request Bug fix labels Jul 30, 2024
    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🏅 Score: 85
    🧪 No relevant tests
    🔒 No security concerns identified
    🔀 No multiple PR themes
    ⚡ Key issues to review

    Error Handling
    The error handling in get_repo_settings might suppress exceptions other than HTTPError which could be important for debugging. Consider logging all exceptions or re-raising them after logging.

    Exception Handling
    In publish_inline_comment, the method re-raises the caught exception which is good for transparency, but ensure that this is consistent with your error handling strategy across your application.

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error handling to _get_pr_comments_path to ensure all required parameters are present

    Implement error handling for the new method _get_pr_comments_path to manage
    potential issues with URL construction, such as missing or None values in
    workspace_slug or repo_slug.

    pr_agent/git_providers/bitbucket_server_provider.py [407-408]

     def _get_pr_comments_path(self):
    +    if not self.workspace_slug or not self.repo_slug or not self.pr_num:
    +        raise ValueError("Missing required parameters for constructing the PR comments path")
         return f"rest/api/latest/projects/{self.workspace_slug}/repos/{self.repo_slug}/pull-requests/{self.pr_num}/comments"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Implementing error handling for _get_pr_comments_path ensures that all required parameters are present, preventing potential runtime errors.

    9
    Best practice
    Replace broad exception handling with specific exceptions

    Replace the broad Exception handling with more specific exceptions to avoid catching
    unintended exceptions. This will make the error handling more precise and easier to
    debug.

    pr_agent/git_providers/bitbucket_server_provider.py [46]

    -except Exception as e:
    +except (HTTPError, AnotherSpecificException) as e:
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Replacing broad exception handling with specific exceptions improves error handling precision and debugging, which is a best practice.

    8
    Use a context manager for better resource management in exception handling

    Use a context manager when handling exceptions to ensure that all resources are
    properly managed even when an error occurs.

    pr_agent/git_providers/bitbucket_server_provider.py [240-243]

    -try:
    -    self.bitbucket_client.post(self._get_pr_comments_path(), data=payload)
    -except Exception as e:
    -    get_logger().error(f"Failed to publish inline comment to '{file}' at line {from_line}, error: {e}")
    -    raise e
    +with self.bitbucket_client.post(self._get_pr_comments_path(), data=payload) as response:
    +    try:
    +        response.raise_for_status()
    +    except HTTPError as e:
    +        get_logger().error(f"Failed to publish inline comment to '{file}' at line {from_line}, error: {e}")
    +        raise e
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a context manager ensures proper resource management, but the current code does not seem to require this level of complexity.

    6
    Enhancement
    Use None to represent the absence of data instead of an empty string

    Instead of returning an empty string when the file is not found, consider using None
    to represent the absence of data. This can help distinguish between an empty file
    and a file not found scenario.

    pr_agent/git_providers/bitbucket_server_provider.py [48]

    -return ""
    +return None
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using None to represent the absence of data can help distinguish between an empty file and a file not found scenario, enhancing code clarity.

    7

    @Codium-ai Codium-ai deleted a comment from mrT23 Jul 31, 2024
    @Codium-ai Codium-ai deleted a comment from mrT23 Jul 31, 2024
    @mrT23
    Copy link
    Collaborator

    mrT23 commented Jul 31, 2024

    @MarkRx
    Thanks for the feedback. we are investigating. does seem like a problem

    @mrT23 mrT23 merged commit 9560bc1 into Codium-ai:main Aug 1, 2024
    1 check passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants