Feature/bwdo 520 review #3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new review feature (BWDO-520) that integrates source control review functionality with ticketing systems (Jira and Redmine) and Git hosting providers (GitHub and GitLab). The feature allows users to add commit/PR information to tickets and manage VCS and ticketing instances through a CLI.
Key Changes:
- Added CLI commands for review operations including
review,add_git_instance, andadd_ticketing_instance - Implemented ticketing system integrations for Jira and Redmine with CRUD operations
- Implemented Git hosting provider integrations for GitHub and GitLab with PR management
- Created configuration management for ticketing and Git instances
Reviewed Changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 29 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sc/review_cli.py | New CLI module exposing review commands |
| src/sc/review/ticketing_instances/ticketing_instance.py | Abstract base class for ticketing system implementations |
| src/sc/review/ticketing_instances/ticket_instance_factory.py | Factory for creating ticketing instance objects |
| src/sc/review/ticketing_instances/ticket.py | Data class representing a ticket |
| src/sc/review/ticketing_instances/redmine_instance.py | Redmine ticketing system implementation |
| src/sc/review/ticketing_instances/jira_instance.py | Jira ticketing system implementation |
| src/sc/review/review_config.py | Configuration management for review feature |
| src/sc/review/review.py | Core review logic for processing repos and tickets |
| src/sc/review/main.py | Entry points for review commands with user interaction |
| src/sc/review/git_instances/pull_request.py | Data class for pull request representation |
| src/sc/review/git_instances/gitlab_instance.py | GitLab API integration |
| src/sc/review/git_instances/github_instance.py | GitHub API integration |
| src/sc/review/git_instances/git_instance.py | Abstract base class for Git hosting providers |
| src/sc/review/git_instances/git_factory.py | Factory for creating Git instance objects |
| src/sc/review/exceptions.py | Custom exceptions for review operations |
| src/sc/cli.py | Updated to import and register review CLI commands |
| setup.py | Added dependencies for jira and python-redmine libraries |
Comments suppressed due to low confidence (4)
src/sc/review/ticketing_instances/redmine_instance.py:200
- Missing named argument for string format. Format "{url}/issues/{ticket_number}" requires 'ticket_number', but it is omitted.
'{url}/issues/{ticket_number}'.format(url=self._url),
src/sc/review/ticketing_instances/redmine_instance.py:40
- Mixing implicit and explicit returns may indicate an error, as implicit returns always return None.
def validate_connection(self) -> bool:
src/sc/review/ticketing_instances/redmine_instance.py:76
- Keyword argument 'additional_information' is not a supported parameter name of TicketingInstanceUnreachable.init.
raise TicketingInstanceUnreachable(
self.url,
additional_information=''.join(
arg for arg in exception.args)) from exception
src/sc/review/ticketing_instances/redmine_instance.py:48
- Instantiating an exception, but not raising it, has no effect.
ConnectionError("Invalid Redmine API key or insufficient permssions.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Creates a ticket on the Jira instance | ||
| Args: | ||
| project_key (str): The projects key on th Jira instance |
There was a problem hiding this comment.
Typo in docstring: "th Jira" should be "the Jira".
| project_key (str): The projects key on th Jira instance | |
| project_key (str): The project's key on the Jira instance |
src/sc/review/main.py
Outdated
| logger.error(f"Failed to connect! {e}") | ||
| sys.exit(1) | ||
|
|
||
| logger.info("Connection succesful!") |
There was a problem hiding this comment.
Typo in log message: "succesful" should be "successful".
| logger.info("Connection succesful!") | |
| logger.info("Connection successful!") |
| output_location: str = None, | ||
| obfuscate: bool = False): | ||
| filepath = abspath(file_location) if exists(file_location) else None | ||
| if filepath is None and self._downloader: |
There was a problem hiding this comment.
The _get_file method references self._downloader on line 157, but this attribute is never initialized in the __init__ method or anywhere else in the class. This will cause an AttributeError when the method is called.
src/sc/review/main.py
Outdated
|
|
||
| if provider == "github": | ||
| url = "http://api.github.com" | ||
| logger.info("Enter a pattern for to identify Git from remote url: ") |
There was a problem hiding this comment.
Typo in log message: "for to identify" should be "to identify" (remove the extra "for").
| logger.info("Enter a pattern for to identify Git from remote url: ") | |
| logger.info("Enter a pattern to identify Git from remote url: ") |
src/sc/review/review.py
Outdated
| f"Author: [{data.commit_author}]", | ||
| f"Date: [{data.commit_date}]", | ||
| "", | ||
| f"{data.commit_message}" |
There was a problem hiding this comment.
Missing comma after line 349, causing the string "</pre>" on line 350 to be concatenated with the previous line instead of being a separate element in the tuple.
| f"{data.commit_message}" | |
| f"{data.commit_message}", |
| def create_ticket(self) -> Ticket: | ||
| """Abstract Method: | ||
| Create a ticket on the ticketing instance. | ||
| Read it's contents return the new ticket object |
There was a problem hiding this comment.
Typo in docstring: "it's contents" should be "its contents" (possessive form, no apostrophe).
| Read it's contents return the new ticket object | |
| Read its contents return the new ticket object |
| url=self._instance.url, ticket_number=self.ticket_id) | ||
| try: | ||
| self._instance.issue.delete(self.ticket_id) |
There was a problem hiding this comment.
The delete_ticket method uses self.ticket_id instead of the ticket_id parameter passed to the function. This will cause an AttributeError since self.ticket_id is not defined on the instance. Use the ticket_id parameter instead.
| url=self._instance.url, ticket_number=self.ticket_id) | |
| try: | |
| self._instance.issue.delete(self.ticket_id) | |
| url=self._instance.url, ticket_number=ticket_id) | |
| try: | |
| self._instance.issue.delete(ticket_id) |
| raise TicketNotFound(issue_url) from exception | ||
| except ForbiddenError as exception: | ||
| raise PermissionsError( | ||
| '{url}/issues/{ticket_number}'.format(url=self._url), |
There was a problem hiding this comment.
Inconsistent error message formatting: line 200 uses self._url without the ticket_number placeholder, unlike line 193. This should match the pattern used in line 192-193 for consistency.
| '{url}/issues/{ticket_number}'.format(url=self._url), | |
| issue_url, |
| def create_ticket(self, project_key: str, issue_type: str, title: str, | ||
| **kwargs): | ||
| """Creates a ticket on the Jira instance | ||
| Args: | ||
| project_key (str): The projects key on th Jira instance | ||
| issue_type (str): The type of issue to raise | ||
| titlr (str): Title of the ticket to raise | ||
| """ | ||
| project_dict = {'project': {'key': project_key}} | ||
| kwargs.update(project_dict) | ||
| kwargs.update({'issuetype': issue_type, 'summary': title}) | ||
| new_issue_id = self._instance.create_issue(fields=kwargs).key |
There was a problem hiding this comment.
This method requires 4 positional arguments, whereas overridden TicketingInstance.create_ticket requires 1.
| def create_ticket(self, project_key: str, issue_type: str, title: str, | |
| **kwargs): | |
| """Creates a ticket on the Jira instance | |
| Args: | |
| project_key (str): The projects key on th Jira instance | |
| issue_type (str): The type of issue to raise | |
| titlr (str): Title of the ticket to raise | |
| """ | |
| project_dict = {'project': {'key': project_key}} | |
| kwargs.update(project_dict) | |
| kwargs.update({'issuetype': issue_type, 'summary': title}) | |
| new_issue_id = self._instance.create_issue(fields=kwargs).key | |
| def create_ticket(self, ticket_data): | |
| """Creates a ticket on the Jira instance | |
| Args: | |
| ticket_data (dict): Dictionary containing ticket fields. Must include | |
| 'project_key', 'issue_type', and 'title'. | |
| """ | |
| project_key = ticket_data.get('project_key') | |
| issue_type = ticket_data.get('issue_type') | |
| title = ticket_data.get('title') | |
| # Remove these keys from ticket_data to avoid duplication | |
| fields = {k: v for k, v in ticket_data.items() if k not in ('project_key', 'issue_type', 'title')} | |
| project_dict = {'project': {'key': project_key}} | |
| fields.update(project_dict) | |
| fields.update({'issuetype': issue_type, 'summary': title}) | |
| new_issue_id = self._instance.create_issue(fields=fields).key |
| def create_ticket(self, project_name: str, title: str, **kwargs) -> Ticket: | ||
| """Create a ticket on the redmine instance | ||
| Args: | ||
| project_name (str): The project to create the ticket under | ||
| title (str): The title of the ticket | ||
| Raises: | ||
| PermissionsError: User does not have permission to access the ticket on the instances | ||
| TicketingInstanceUnreachable: The redmine instance is unreachable | ||
| """ |
There was a problem hiding this comment.
This method requires 3 positional arguments, whereas overridden TicketingInstance.create_ticket requires 1.
| def create_ticket(self, project_name: str, title: str, **kwargs) -> Ticket: | |
| """Create a ticket on the redmine instance | |
| Args: | |
| project_name (str): The project to create the ticket under | |
| title (str): The title of the ticket | |
| Raises: | |
| PermissionsError: User does not have permission to access the ticket on the instances | |
| TicketingInstanceUnreachable: The redmine instance is unreachable | |
| """ | |
| def create_ticket(self, title: str, **kwargs) -> Ticket: | |
| """Create a ticket on the redmine instance | |
| Args: | |
| title (str): The title of the ticket | |
| project_name (str, in kwargs): The project to create the ticket under | |
| Raises: | |
| PermissionsError: User does not have permission to access the ticket on the instances | |
| TicketingInstanceUnreachable: The redmine instance is unreachable | |
| """ | |
| project_name = kwargs.pop('project_name', None) | |
| if not project_name: | |
| raise ValueError("Missing required argument: 'project_name'") |
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
56ca670 to
fb132d5
Compare
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
fb132d5 to
87201bf
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
src/sc/review/ticketing_instances/redmine_instance.py:200
- Missing named argument for string format. Format "{url}/issues/{ticket_number}" requires 'ticket_number', but it is omitted.
'{url}/issues/{ticket_number}'.format(url=self._url),
src/sc/review/ticketing_instances/redmine_instance.py:48
- Instantiating an exception, but not raising it, has no effect.
ConnectionError("Invalid Redmine API key or insufficient permssions.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| comment_message (str): The message to add as a comment | ||
| ticket_id (str): The ticket number to add the comment to. | ||
| """ | ||
| ticket = self.update_ticket(ticket_id,notes=self._convert_html_colours(comment_message)) |
There was a problem hiding this comment.
Missing space after comma in function call. notes=self._convert_html_colours(comment_message) should have a space: notes=self._convert_html_colours(comment_message). While Python allows this, PEP 8 recommends spaces around the = in keyword arguments only when not following a comma, but there should be a space after the comma before notes.
| ticket = self.update_ticket(ticket_id,notes=self._convert_html_colours(comment_message)) | |
| ticket = self.update_ticket(ticket_id, notes=self._convert_html_colours(comment_message)) |
| raise TicketNotFound(issue_url) from exception | ||
| except ForbiddenError as exception: | ||
| raise PermissionsError( | ||
| '{url}/issues/{ticket_number}'.format(url=self._url), |
There was a problem hiding this comment.
Incomplete URL formatting. Line 200 references {ticket_number} in the format string but doesn't provide this variable - it should use ticket_id parameter. This will cause a KeyError.
| '{url}/issues/{ticket_number}'.format(url=self._url), | |
| '{url}/issues/{ticket_number}'.format(url=self._url, ticket_number=ticket_id), |
| # If the user does not want to use SSL verification disable the warnings about it being insecure. | ||
| if self._default_jira_setup.get('verify') is False: | ||
| disable_warnings(InsecureRequestWarning) | ||
| # 9.0.0. is an arbitrary version that hasn't been release yet |
There was a problem hiding this comment.
Typo in comment. "release" should be "released".
| # 9.0.0. is an arbitrary version that hasn't been release yet | |
| # 9.0.0. is an arbitrary version that hasn't been released yet |
| r = requests.get(url, headers=self._headers(), params=params, timeout=10) | ||
| r.raise_for_status() | ||
| prs = r.json() | ||
| print(r.json()) |
There was a problem hiding this comment.
Debug print statement left in production code. Line 37 contains print(r.json()) which should be removed or replaced with proper logging.
| print(r.json()) |
src/sc/review/review_config.py
Outdated
| def get_ticket_host_data(self, identifier: str) -> TicketHostCfg: | ||
| data = self._ticket_config.get_config().get(identifier) | ||
| if not data: | ||
| raise KeyError(f"Git instance config doesn't contain entry for {identifier}") |
There was a problem hiding this comment.
Incorrect error message in KeyError. The message says "Git instance config doesn't contain entry for {identifier}" but this method is for ticket host configuration, not git instance configuration. Should say "Ticket host config doesn't contain entry for {identifier}".
| raise KeyError(f"Git instance config doesn't contain entry for {identifier}") | |
| raise KeyError(f"Ticket host config doesn't contain entry for {identifier}") |
| f"Date: [{data.commit_date}]", | ||
| "", | ||
| f"{data.commit_message}" | ||
| "</pre>" |
There was a problem hiding this comment.
Missing comma in tuple. Line 351 should end with a comma to maintain tuple structure consistency. The closing </pre> should be "</pre>" (with comma after).
| "</pre>" | |
| "</pre>", |
| def engine(self) -> str: | ||
| return 'redmine' | ||
|
|
||
| def validate_connection(self) -> bool: |
There was a problem hiding this comment.
Mixing implicit and explicit returns may indicate an error, as implicit returns always return None.
| additional_information=''.join( | ||
| arg for arg in exception.args)) from exception |
There was a problem hiding this comment.
Keyword argument 'additional_information' is not a supported parameter name of TicketingInstanceUnreachable.init.
| additional_information=''.join( | |
| arg for arg in exception.args)) from exception | |
| ''.join(arg for arg in exception.args) | |
| ) from exception |
| def get_pull_request(self, repo: str, source_branch: str) -> PullRequest: | ||
| pass | ||
|
|
||
| @abstractmethod | ||
| def get_create_pr_url( |
There was a problem hiding this comment.
Might be better to review to PRs at code_reviews, for a more instance agnostic approach. Since gitlab refers them as merge_requests and PRs are a github thing.
There was a problem hiding this comment.
I'd have this file and the git_instances folder. Since this should just import all instance types from the folder and is the only to be imported from the files above. Then have an init.py file in the git_instance folder to only expose the implemented instances and not the abstract instance.
| logger.info( | ||
| "E.G. github.com for all github instances or " | ||
| "github.com/org for a particular organisation") |
There was a problem hiding this comment.
I think we should revisit this at a later date. I don't think this is a particularly intuitive way to do it, but it will work fine for now.
| instance = GitFactory.create(provider, api_key, url) | ||
|
|
||
| try: | ||
| instance.validate_connection() |
There was a problem hiding this comment.
Why don't we just do this as part of the create from GitFactory?
|
|
||
| logger.info("Connection validated!") | ||
|
|
||
| git_cfg = GitInstanceCfg(url=url, token=api_key, provider=provider) |
There was a problem hiding this comment.
Can we not have this come from the instance, since we just passed all this data in there, so it should know it's configuration.
src/sc/review/review.py
Outdated
| identifier, ticket_num = self._match_branch( | ||
| branch_name, | ||
| ticket_host_ids | ||
| ) |
There was a problem hiding this comment.
Why not stick the self._config.get_ticket_host_ids() into the _match_branch function and knock out the ticket_host_ids arg.
src/sc/review/review.py
Outdated
| ticketing_instance, ticketing_cfg = self._load_ticketing(identifier) | ||
|
|
||
| ticket_id = f"{ticketing_cfg.project_prefix or ''}{ticket_num}" | ||
| ticket = ticketing_instance.read_ticket(ticket_id) |
There was a problem hiding this comment.
Should this be wrapped in a try/except to catch when it throws errors and handle them nicely.
src/sc/review/review.py
Outdated
| logger.warning(e) | ||
| identifier, ticket_num = None, None | ||
|
|
||
| if identifier and ticket_num: |
There was a problem hiding this comment.
You can avoid this if statement if you just used the try/except above as the if/else
| manifest_repo, manifest_git) | ||
| comments.append(comment_data) | ||
|
|
||
| print(self._generate_combined_terminal_comment(comments)) |
There was a problem hiding this comment.
Does it still print out in colour in the terminal?
There was a problem hiding this comment.
Move this out of the ticketing_instances dir
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return True | ||
|
|
||
| except (AuthError, ForbiddenError) as e: | ||
| raise ConnectionError("Invalid Redmine API key or insufficient permssions.") |
There was a problem hiding this comment.
Corrected spelling of 'permssions' to 'permissions'.
| raise ConnectionError("Invalid Redmine API key or insufficient permssions.") | |
| raise ConnectionError("Invalid Redmine API key or insufficient permissions.") |
| Args: | ||
| project_key (str): The project's key on the Jira instance | ||
| issue_type (str): The type of issue to raise | ||
| titlr (str): Title of the ticket to raise |
There was a problem hiding this comment.
Corrected spelling of 'titlr' to 'title'.
| titlr (str): Title of the ticket to raise | |
| title (str): Title of the ticket to raise |
src/sc/review/factories/__init__.py
Outdated
| @@ -0,0 +1,2 @@ | |||
| from .git_factory import GitFactory | |||
| from . ticket_instance_factory import TicketingInstanceFactory No newline at end of file | |||
There was a problem hiding this comment.
There's a space after the dot in the import statement. It should be from .ticket_instance_factory without the space.
| from . ticket_instance_factory import TicketingInstanceFactory | |
| from .ticket_instance_factory import TicketingInstanceFactory |
src/sc/review/main.py
Outdated
| print("") | ||
|
|
||
| if provider == "github": | ||
| url = "http://api.github.com" |
There was a problem hiding this comment.
GitHub API URL should use HTTPS protocol. This should be https://api.github.com to match the default in GithubInstance (line 8 of github_instance.py) and to ensure secure communication.
| url = "http://api.github.com" | |
| url = "https://api.github.com" |
src/sc/review/main.py
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Missing sys.exit(1) or return after error message. If an invalid provider is entered, the code continues execution and will fail with NameError on line 60 when trying to use undefined variables url and pattern.
| sys.exit(1) |
| def create_ticket(self, project_name: str, title: str, **kwargs) -> Ticket: | ||
| """Create a ticket on the redmine instance | ||
| Args: | ||
| project_name (str): The project to create the ticket under | ||
| title (str): The title of the ticket | ||
| Raises: | ||
| PermissionsError: User does not have permission to access the ticket on the instances | ||
| TicketingInstanceUnreachable: The redmine instance is unreachable | ||
| """ |
There was a problem hiding this comment.
This method requires 3 positional arguments, whereas overridden TicketingInstance.create_ticket requires 1.
| def create_ticket(self, project_name: str, title: str, **kwargs) -> Ticket: | |
| """Create a ticket on the redmine instance | |
| Args: | |
| project_name (str): The project to create the ticket under | |
| title (str): The title of the ticket | |
| Raises: | |
| PermissionsError: User does not have permission to access the ticket on the instances | |
| TicketingInstanceUnreachable: The redmine instance is unreachable | |
| """ | |
| def create_ticket(self, ticket_data) -> Ticket: | |
| """Create a ticket on the redmine instance | |
| Args: | |
| ticket_data (dict): Dictionary containing ticket fields, must include 'project_name' and 'title' | |
| Raises: | |
| PermissionsError: User does not have permission to access the ticket on the instances | |
| TicketingInstanceUnreachable: The redmine instance is unreachable | |
| """ | |
| project_name = ticket_data.get('project_name') | |
| title = ticket_data.get('title') | |
| # Remove project_name and title from ticket_data to avoid passing them twice | |
| kwargs = {k: v for k, v in ticket_data.items() if k not in ('project_name', 'title')} |
| @@ -0,0 +1,150 @@ | |||
| #!/usr/bin/env python3 | |||
| from abc import ABC, abstractmethod | |||
| from os.path import abspath, exists | |||
There was a problem hiding this comment.
Import of 'abspath' is not used.
Import of 'exists' is not used.
| from os.path import abspath, exists |
|
Hi @BenjiMilan : Please add headers to all new code files (except very small 1 or 2 liners). |
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
9b7241a to
1a387bf
Compare
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
1a387bf to
cfb26b2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 20 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| comment = self._convert_from_html(comment_message) | ||
| ticket = self._instance.add_comment(ticket_id, comment=comment) | ||
| ticket = self._instance.up |
There was a problem hiding this comment.
This line appears to be incomplete. The code attempts to call self._instance.up which is not a valid method or attribute access. This will cause an AttributeError at runtime.
| ticket = self._instance.up | |
| ticket = self.read_ticket(ticket_id) |
| class RedmineInstance(TicketingInstance): | ||
| """ | ||
| A class to handle operations on Redmine tickets. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| instance: Redmine | ||
| ): | ||
| self._instance = instance | ||
|
|
||
| @property | ||
| def engine(self) -> str: | ||
| return 'redmine' | ||
|
|
||
| def validate_connection(self) -> bool: | ||
| """Check if the Redmine instance and API key are valid. | ||
| Raises: | ||
| ConnectionError: If the connection is invalid. | ||
| Returns: | ||
| bool: True if connection is valid. | ||
| """ | ||
| try: | ||
| # Minimal authenticated request | ||
| self._instance.auth() # redminelib verifies credentials | ||
| return True | ||
|
|
||
| except (AuthError, ForbiddenError) as e: | ||
| raise ConnectionError("Invalid Redmine API key or insufficient permssions.") | ||
|
|
||
| except BaseRedmineError as e: | ||
| raise ConnectionError(f"Redmine API error: {e}") | ||
|
|
||
| except RequestException as e: | ||
| raise ConnectionError("Failed to reach Redmine server.") from e | ||
|
|
||
| def read_ticket(self, ticket_id: str) -> Ticket: | ||
| """Read the information from a ticket and put it's contents in this object contents dict | ||
| Args: | ||
| ticket_id (str): The ticket number to read. | ||
| Raises: | ||
| TicketNotFound: Ticket not found on the redmine instance | ||
| PermissionsError: User does not have permission to access the ticket on the instances | ||
| TicketingInstanceUnreachable: The redmine instance is unreachable | ||
| """ | ||
| ticket_url = self.url + '/issues/' + ticket_id | ||
| try: | ||
| issue = self._instance.issue.get(ticket_id, include=['journals']) | ||
| except ResourceNotFoundError as exception: | ||
| raise TicketNotFound(ticket_url) from exception | ||
| except (AuthError, ForbiddenError) as exception: | ||
| raise PermissionsError( | ||
| ticket_url, | ||
| 'Please contact the dev-support team') from exception | ||
| except SSLError as exception: | ||
| raise TicketingInstanceUnreachable( | ||
| ticket_url, | ||
| additional_info=''.join(str(arg) for arg in exception.args)) | ||
| issue_contents = dict((k, v) for k, v in list(issue)) | ||
| author = issue_contents['author'].get('name') | ||
| try: | ||
| assignee = issue_contents['assigned_to'].get('name') | ||
| except KeyError: | ||
| assignee = None | ||
| comments = issue_contents.get('journals') | ||
| status = issue_contents['status'].get('name') | ||
| try: | ||
| target_version = issue_contents['fixed_version'].get('name') | ||
| except KeyError: | ||
| target_version = None | ||
| title = issue_contents.get('subject') | ||
| ticket = Ticket(ticket_url, | ||
| author=author, | ||
| assignee=assignee, | ||
| comments=comments, | ||
| contents=issue_contents, | ||
| id=ticket_id, | ||
| status=status, | ||
| title=title, | ||
| url=ticket_url, | ||
| target_version=target_version) | ||
| return ticket | ||
|
|
||
| def update_ticket(self, ticket_id: str, **kwargs): | ||
| """Writes the changed fields from the kwargs, back to the ticket | ||
| Raises: | ||
| TicketNotFound: Ticket not found on the redmine instance | ||
| PermissionsError: User does not have permission to access the ticket on the instances | ||
| TicketingInstanceUnreachable: The redmine instance is unreachable | ||
| """ | ||
| issue_url = f'{self.url}/issues/{ticket_id}' | ||
| try: | ||
| self._instance.issue.update(ticket_id, **kwargs) | ||
| except ResourceNotFoundError as exception: | ||
| raise TicketNotFound(issue_url) from exception | ||
| except ForbiddenError as exception: | ||
| raise PermissionsError( | ||
| issue_url, 'Please contact the dev-support team') from exception | ||
| except SSLError as exception: | ||
| raise TicketingInstanceUnreachable( | ||
| issue_url, | ||
| additional_info=''.join(str(arg) for arg in exception.args)) | ||
| ticket = self.read_ticket(ticket_id) | ||
| return ticket | ||
|
|
||
| def add_comment_to_ticket(self, ticket_id: str, comment_message: str) -> None: | ||
| """Add a comment to a ticket on the redmine instance | ||
| Args: | ||
| comment_message (str): The message to add as a comment | ||
| ticket_id (str): The ticket number to add the comment to. | ||
| """ | ||
| ticket = self.update_ticket( | ||
| ticket_id, notes=self._convert_html_colours(comment_message)) | ||
| return ticket | ||
|
|
||
| def _convert_html_colours(self, string: str) -> str: | ||
| """Convert a html colour tags to redmine formatted colour tags. | ||
| Args: | ||
| string (str): html formatted string. | ||
| Returns: | ||
| str: Input string with html colour tags converted to redmine colour tags. | ||
| """ | ||
| string = string.replace('<p style="', r'%{') | ||
| string = string.replace('">', '}') | ||
| string = string.replace('</p>', r'%') | ||
| return string |
There was a problem hiding this comment.
The RedmineInstance class doesn't implement the abstract methods create_ticket and delete_ticket required by the TicketingInstance base class. This will cause errors if these methods are called.
| git_instance = self._create_git_instance(Repo(self.top_dir).remote().url) | ||
| comment_data = self._create_comment_data(Repo(self.top_dir), git_instance) |
There was a problem hiding this comment.
Missing error handling: Repo(self.top_dir).remote().url assumes a default remote exists. If no remote is configured or the default remote is not named 'origin', this will raise an exception. Consider using Repo(self.top_dir).remotes[0].url with proper validation or specifying the remote name explicitly.
| git_instance = self._create_git_instance(Repo(self.top_dir).remote().url) | |
| comment_data = self._create_comment_data(Repo(self.top_dir), git_instance) | |
| repo = Repo(self.top_dir) | |
| # Try to get the remote URL safely | |
| if not repo.remotes: | |
| raise RemoteUrlNotFound(f"No remotes found for repository at {self.top_dir}") | |
| # Prefer 'origin' if it exists, else use the first remote | |
| remote = repo.remotes.origin if 'origin' in repo.remotes else repo.remotes[0] | |
| git_instance = self._create_git_instance(remote.url) | |
| comment_data = self._create_comment_data(repo, git_instance) |
| instance: JIRA | ||
| ): | ||
| self._instance = instance |
There was a problem hiding this comment.
The JiraInstance class is missing the url property, but it's being used in methods (e.g., line 93, 122). This will cause an AttributeError at runtime.
| instance: JIRA | |
| ): | |
| self._instance = instance | |
| instance: JIRA, | |
| url: str | |
| ): | |
| self._instance = instance | |
| self.url = url |
| def c(code, text): | ||
| return f"\033[{code}m{text}\033[0m" |
There was a problem hiding this comment.
[nitpick] The function name c is too short and unclear. Consider renaming it to something more descriptive like colorize or add_ansi_color to better indicate its purpose of adding ANSI color codes.
| raise ConnectionError("Failed to reach Redmine server.") from e | ||
|
|
||
| def read_ticket(self, ticket_id: str) -> Ticket: | ||
| """Read the information from a ticket and put it's contents in this object contents dict |
There was a problem hiding this comment.
The docstring states "it's contents" but should be "its contents" (no apostrophe for possessive).
| """Read the information from a ticket and put it's contents in this object contents dict | |
| """Read the information from a ticket and put its contents in this object contents dict |
| """ | ||
| try: | ||
| comment = self._convert_from_html(comment_message) | ||
| ticket = self._instance.add_comment(ticket_id, comment=comment) |
There was a problem hiding this comment.
This assignment to 'ticket' is unnecessary as it is redefined before this value is used.
This assignment to 'ticket' is unnecessary as it is redefined before this value is used.
| ticket = self._instance.add_comment(ticket_id, comment=comment) | |
| self._instance.add_comment(ticket_id, comment=comment) |
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from json.decoder import JSONDecodeError |
There was a problem hiding this comment.
Import of 'JSONDecodeError' is not used.
| from json.decoder import JSONDecodeError |
|
|
||
| from json.decoder import JSONDecodeError | ||
| from requests import post | ||
| from requests.exceptions import InvalidURL, RequestException |
There was a problem hiding this comment.
Import of 'InvalidURL' is not used.
| from requests.exceptions import InvalidURL, RequestException | |
| from requests.exceptions import RequestException |
| from jira import JIRA | ||
| from jira.exceptions import JIRAError | ||
|
|
||
| from ..exceptions import PermissionsError, TicketingInstanceUnreachable, TicketNotFound |
There was a problem hiding this comment.
Import of 'TicketingInstanceUnreachable' is not used.
| from ..exceptions import PermissionsError, TicketingInstanceUnreachable, TicketNotFound | |
| from ..exceptions import PermissionsError, TicketNotFound |
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
2f97745 to
3923af5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 19 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/sc/review/main.py
Outdated
| print("") | ||
|
|
||
| if provider == "github": | ||
| url = "http://api.github.com" |
There was a problem hiding this comment.
The GitHub API URL should use HTTPS instead of HTTP for security. The correct URL is "https://api.github.com".
src/sc/review/factories/__init__.py
Outdated
| @@ -0,0 +1,2 @@ | |||
| from .git_factory import GitFactory | |||
| from . ticket_instance_factory import TicketingInstanceFactory No newline at end of file | |||
There was a problem hiding this comment.
Incorrect spacing in import statement. There should be no space between the dot and "ticket_instance_factory".
| def get_code_review(self, repo: str, source_branch: str) -> CodeReview: | ||
| safe_repo = urllib.parse.quote(repo, safe='') | ||
| url = f"{self.base_url}/api/v4/projects/{safe_repo}/merge_requests" | ||
| params = {"state": "all", "source_branch": source_branch} | ||
| r = requests.get(url, headers=self._headers(), params=params, timeout=10) | ||
| r.raise_for_status() |
There was a problem hiding this comment.
The get_code_review method doesn't handle potential network errors. If requests.get raises an exception (timeout, connection error, HTTP error), it will propagate uncaught. Consider adding try-except blocks similar to the validate_connection method to provide meaningful error messages.
src/sc/review/review.py
Outdated
|
|
||
| input_id = input("> ") | ||
| while input_id not in ticket_conf.keys(): | ||
| logger.info("Prefix {input_id} not found in instances.") |
There was a problem hiding this comment.
Missing f-string prefix. The string contains {input_id} but isn't an f-string, so it will print the literal text "Prefix {input_id} not found in instances." instead of interpolating the variable.
| def get_code_review(self, repo: str, source_branch: str) -> CodeReview | None: | ||
| url = f"{self.base_url}/repos/{repo}/pulls" | ||
| owner = repo.split("/")[0] | ||
| params = {"state": "all", "head": f"{owner}:{source_branch}"} | ||
| r = requests.get(url, headers=self._headers(), params=params, timeout=10) | ||
| r.raise_for_status() |
There was a problem hiding this comment.
The get_code_review method doesn't handle potential network errors. If requests.get raises an exception (timeout, connection error, HTTP error), it will propagate uncaught. Consider adding try-except blocks similar to the validate_connection method to provide meaningful error messages.
| ticketing_instance.add_comment_to_ticket(ticket_id, ticket_comment) | ||
|
|
||
| def run_repo_command(self): | ||
| manifest_repo = Repo(self.top_dir / '.repo' / 'manifests') |
There was a problem hiding this comment.
Type inconsistency. The init method accepts top_dir as a str, but in run_repo_command (line 81), it's used with Path division operator (/), which requires top_dir to be a Path object. This will raise a TypeError. Either convert top_dir to Path in init (self.top_dir = Path(top_dir)) or use Path(self.top_dir) when performing path operations.
| logger.info(f"Ticket URL: [{ticket.url if ticket else ''}]") | ||
| logger.info("Ticket info: ") | ||
|
|
||
| manifest = ScManifest.from_repo_root(self.top_dir / '.repo') |
There was a problem hiding this comment.
Type inconsistency. The init method accepts top_dir as a str, but this line uses Path division operator (/), which requires top_dir to be a Path object. This will raise a TypeError. Either convert top_dir to Path in init (self.top_dir = Path(top_dir)) or use Path(self.top_dir) when performing path operations.
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| import urllib3 |
There was a problem hiding this comment.
Import of 'urllib3' is not used.
| from ..ticketing_instances import JiraInstance, RedmineInstance | ||
| from ..core.ticketing_instance import TicketingInstance | ||
|
|
||
| from jira import JIRA |
There was a problem hiding this comment.
Import of 'JIRA' is not used.
| from ..core.ticketing_instance import TicketingInstance | ||
|
|
||
| from jira import JIRA | ||
| from redminelib import Redmine |
There was a problem hiding this comment.
Import of 'Redmine' is not used.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return True | ||
|
|
||
| except (AuthError, ForbiddenError) as e: | ||
| raise ConnectionError("Invalid Redmine API key or insufficient permssions.") |
There was a problem hiding this comment.
Corrected spelling of 'permssions' to 'permissions'.
| raise ConnectionError("Invalid Redmine API key or insufficient permssions.") | |
| raise ConnectionError("Invalid Redmine API key or insufficient permissions.") |
| def _create_ticketing_instance(self, cfg: TicketHostCfg) -> TicketingInstance: | ||
| inst = TicketingInstanceFactory.create( | ||
| provider=cfg.provider, | ||
| url=cfg.url, | ||
| token=cfg.api_key, | ||
| auth_type=cfg.auth_type, | ||
| username=cfg.username, | ||
| cert=cfg.cert |
There was a problem hiding this comment.
The parameter name cfg is ambiguous. It should be renamed to ticket_host_cfg or config for clarity.
| def _create_ticketing_instance(self, cfg: TicketHostCfg) -> TicketingInstance: | |
| inst = TicketingInstanceFactory.create( | |
| provider=cfg.provider, | |
| url=cfg.url, | |
| token=cfg.api_key, | |
| auth_type=cfg.auth_type, | |
| username=cfg.username, | |
| cert=cfg.cert | |
| def _create_ticketing_instance(self, ticket_host_cfg: TicketHostCfg) -> TicketingInstance: | |
| inst = TicketingInstanceFactory.create( | |
| provider=ticket_host_cfg.provider, | |
| url=ticket_host_cfg.url, | |
| token=ticket_host_cfg.api_key, | |
| auth_type=ticket_host_cfg.auth_type, | |
| username=ticket_host_cfg.username, | |
| cert=ticket_host_cfg.cert |
|
|
||
| @abstractmethod | ||
| def get_code_review(self, repo: str, source_branch: str) -> CodeReview: | ||
| """Get information about about a branches code review. |
There was a problem hiding this comment.
The phrase "about about" contains a duplicate word. Should be "about a branch's code review."
| """Get information about about a branches code review. | |
| """Get information about a branch's code review. |
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
66fb6e8 to
8737e84
Compare
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
407885c to
b6a9b80
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 26 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| except (AuthError, ForbiddenError) as e: | ||
| raise ConnectionError( | ||
| "Invalid Redmine API key or insufficient permssions for " |
There was a problem hiding this comment.
Typo: "permssions" should be "permissions".
| "Invalid Redmine API key or insufficient permssions for " | |
| "Invalid Redmine API key or insufficient permissions for " |
| verify_ssl: bool = False | ||
| ): | ||
| if not verify_ssl: | ||
| urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) |
There was a problem hiding this comment.
SSL verification is disabled by default (verify_ssl=False). This is a security risk as it makes the connection vulnerable to man-in-the-middle attacks. Consider making SSL verification enabled by default and requiring explicit opt-out, or at minimum add a prominent warning in the documentation.
| verify_ssl: bool = False | |
| ): | |
| if not verify_ssl: | |
| urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) | |
| verify_ssl: bool = True | |
| ): |
|
|
||
| @cli.command() | ||
| def review(): | ||
| """Add commit/PR information to your ticket.""" |
There was a problem hiding this comment.
The docstring states "Add commit/PR information to your ticket", but this is the same generic description used for all three commands (review, add_git_instance, add_ticketing_instance). The add_git_instance and add_ticketing_instance docstrings correctly describe their purpose. Consider updating the review command's docstring to something more specific like "Add commit and code review information to your ticket".
| """Add commit/PR information to your ticket.""" | |
| """Add commit and code review information to your ticket.""" |
| except requests.Timeout as e: | ||
| raise RuntimeError("Gitlab request timed out") from e | ||
| except requests.HTTPError as e: | ||
| raise RuntimeError( | ||
| f"Gitlab API error {r.status_code}: {r.text}" | ||
| ) from e | ||
| except ValueError as e: # JSON decode error | ||
| raise RuntimeError("Invalid JSON from Gitlab API") from e | ||
| except requests.RequestException as e: |
There was a problem hiding this comment.
The exception handling catches specific requests.Timeout and requests.HTTPError exceptions, but these don't exist in the requests library. The correct exception classes are requests.exceptions.Timeout and requests.exceptions.HTTPError. This will cause an AttributeError at runtime when these exceptions are raised.
| except requests.Timeout as e: | |
| raise RuntimeError("Gitlab request timed out") from e | |
| except requests.HTTPError as e: | |
| raise RuntimeError( | |
| f"Gitlab API error {r.status_code}: {r.text}" | |
| ) from e | |
| except ValueError as e: # JSON decode error | |
| raise RuntimeError("Invalid JSON from Gitlab API") from e | |
| except requests.RequestException as e: | |
| except requests.exceptions.Timeout as e: | |
| raise RuntimeError("Gitlab request timed out") from e | |
| except requests.exceptions.HTTPError as e: | |
| raise RuntimeError( | |
| f"Gitlab API error {r.status_code}: {r.text}" | |
| ) from e | |
| except ValueError as e: # JSON decode error | |
| raise RuntimeError("Invalid JSON from Gitlab API") from e | |
| except requests.exceptions.RequestException as e: |
| raise RuntimeError( | ||
| f"Gitlab API error {r.status_code}: {r.text}" |
There was a problem hiding this comment.
The variable r is referenced in the error handling blocks (lines 57, 59) but may not be defined if the exception occurs during the requests.get() call itself (line 50). This will cause a NameError. Move the status_code and text access into a try-except block or check if r is defined before using it.
| raise RuntimeError( | |
| f"Gitlab API error {r.status_code}: {r.text}" | |
| status_code = e.response.status_code if e.response is not None else "unknown" | |
| response_text = e.response.text if e.response is not None else "" | |
| raise RuntimeError( | |
| f"Gitlab API error {status_code}: {response_text}" |
| if slug.endswith(".git"): | ||
| slug = slug[:-4] |
There was a problem hiding this comment.
The code extracts slug from remote URLs and removes ".git" suffix if present, but doesn't handle the case where ".git" appears elsewhere in the path (e.g., "repo.git.backup.git"). While unlikely, using slug.removesuffix('.git') (Python 3.9+) or slug[:-4] if slug.endswith('.git') else slug would be more robust and only remove the suffix if it's at the end.
| if slug.endswith(".git"): | |
| slug = slug[:-4] | |
| slug = slug.removesuffix(".git") |
|
|
||
| def _create_comment_data(self, repo: Repo, git_instance: GitInstance) -> CommentData: | ||
| branch_name = repo.active_branch.name | ||
| repo_slug = self._get_repo_slug(repo.remotes[0].url) |
There was a problem hiding this comment.
The code uses repo.remotes[0].url without checking if the repository has any remotes. If the repository has no remotes configured, this will raise an IndexError. Consider adding a check for empty remotes or handling the IndexError.
| raise ConnectionError("GitHub API request timed out.") from e | ||
| except requests.exceptions.HTTPError as e: | ||
| status = e.response.status_code | ||
| if status == 400: |
There was a problem hiding this comment.
The error handling checks for status code 400 to indicate an invalid token, but GitHub typically returns 401 for authentication failures, not 400. Status code 400 is for bad requests (malformed request syntax). Consider checking for status codes 401 (Unauthorized) and 403 (Forbidden) like the GitLab implementation does.
| if status == 400: | |
| if status in (401, 403): |
| for identifier in host_identifiers: | ||
| # Matches the identifier, followed by - or _, followed by a number | ||
| if m := re.search(fr'{identifier}[-_]?(\d+)', branch_name): | ||
| ticket_num = m.group(1) | ||
| return identifier, ticket_num |
There was a problem hiding this comment.
The code iterates through all host identifiers and performs a regex search for each one. For repositories with many configured ticketing instances, this could be inefficient. Consider using a single compiled regex pattern that captures all identifiers in one pass, or compile the regex patterns once rather than on each call.
| for identifier in host_identifiers: | |
| # Matches the identifier, followed by - or _, followed by a number | |
| if m := re.search(fr'{identifier}[-_]?(\d+)', branch_name): | |
| ticket_num = m.group(1) | |
| return identifier, ticket_num | |
| # Build a single regex that matches any identifier followed by - or _, then a number. | |
| combined_pattern = re.compile( | |
| rf"({'|'.join(re.escape(ident) for ident in host_identifiers)})[-_]?(\d+)" | |
| ) | |
| m = combined_pattern.search(branch_name) | |
| if m: | |
| identifier = m.group(1) | |
| ticket_num = m.group(2) | |
| return identifier, ticket_num |
| str: The matched pattern. | ||
| """ | ||
| for pattern in git_patterns: | ||
| if pattern in remote_url: |
There was a problem hiding this comment.
The pattern matching for remote URLs uses a simple in check (line 181), which could lead to false positives. For example, if a pattern is "github.com/org1" and the remote URL is "github.com/org123/repo", it would incorrectly match. Consider using more precise pattern matching, such as checking that the pattern is followed by a path separator or end of string.
| if pattern in remote_url: | |
| # Ensure we only match the pattern at a logical boundary, i.e. when | |
| # it is followed by a path separator ("/" or ":") or the end of the string. | |
| if re.search(re.escape(pattern) + r'(?=$|[/:])', remote_url): |
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
de4f061 to
a7a609a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def get_ticketing_config(self) -> dict[str, TicketHostCfg]: | ||
| """Return all ticketing instance configs keyed by identifier.""" | ||
| return {k: TicketHostCfg(**v) for k,v in self._ticket_config.get_config().items()} |
There was a problem hiding this comment.
The return type annotation should be dict[str, TicketHostCfg] but the method returns a dict comprehension where ValidationError exceptions during TicketHostCfg construction could be raised. This could lead to unexpected behavior. Consider handling validation errors gracefully or documenting that this method can raise ValidationError.
| return {k: TicketHostCfg(**v) for k,v in self._ticket_config.get_config().items()} | |
| result: dict[str, TicketHostCfg] = {} | |
| for k, v in self._ticket_config.get_config().items(): | |
| try: | |
| result[k] = TicketHostCfg(**v) | |
| except ValidationError as e: | |
| raise ValueError(f"Invalid config for {k}: {e}") | |
| return result |
| def get_ticket_host_identifiers(self) -> set[str]: | ||
| """Return all configured ticketing instance identifiers.""" | ||
| return self._ticket_config.get_config().keys() |
There was a problem hiding this comment.
The return type is documented as set[str] but dict.keys() returns a dict_keys view object, not a set. While this is often acceptable for iteration, if callers expect set-specific operations, this could cause issues. Consider returning set(self._ticket_config.get_config().keys()) to match the type annotation.
|
|
||
| class RemoteUrlNotFound(ReviewException): | ||
| """Raised when remote url matches no patterns in the config. | ||
| """ |
There was a problem hiding this comment.
The RemoteUrlNotFound exception class is missing a pass statement or docstring body. According to PEP 257, if a class has only a docstring and no methods, it should still have a pass statement for clarity, or the docstring should be on the same line as the class definition if it's a one-liner.
| """ | |
| """ | |
| pass |
| def __init__(self, token: str, base_url: str): | ||
| super().__init__(token, base_url) | ||
| urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) |
There was a problem hiding this comment.
Security concern: SSL verification is disabled globally in the constructor (line 25) and all requests use verify=False (lines 33, 70). This makes the application vulnerable to man-in-the-middle attacks. Consider making SSL verification configurable rather than always disabled, similar to how RedmineInstance handles it with a verify_ssl parameter.
| comment_message (str): The body of the comment | ||
| ticket_id (str, optional): The ticket id to add the comment to. Defaults to None. |
There was a problem hiding this comment.
Documentation inconsistency: The docstring states "ticket_id (str, optional): The ticket id to add the comment to. Defaults to None." However, ticket_id is not an optional parameter (no default value is defined), and passing None would likely cause an error.
| comment_message (str): The body of the comment | |
| ticket_id (str, optional): The ticket id to add the comment to. Defaults to None. | |
| ticket_id (str): The ticket id to add the comment to. | |
| comment_message (str): The body of the comment. |
| Validates if connection to the git instance is successful. | ||
| Raises: | ||
| ConnectionError: If the connection is unsuccesful. |
There was a problem hiding this comment.
The spelling of "unsuccessful" is incorrect. It should be "unsuccessful" not "unsuccesful" (missing an 's').
| ConnectionError: If the connection is unsuccesful. | |
| ConnectionError: If the connection is unsuccessful. |
| return 'redmine' | ||
|
|
||
| def read_ticket(self, ticket_id: str) -> Ticket: | ||
| """Read the information from a ticket and put it's contents in this object contents dict |
There was a problem hiding this comment.
Grammatical error: "it's" should be "its" (possessive, not contraction). The docstring should read "put its contents in this object contents dict".
| """Read the information from a ticket and put it's contents in this object contents dict | |
| """Read the information from a ticket and put its contents in this object contents dict |
src/sc/review/main.py
Outdated
| sys.exit(1) | ||
|
|
||
| logger.info("Enter your api token: ") | ||
| api_key = input("> ") |
There was a problem hiding this comment.
Potential security issue: The API token is read using input() instead of getpass.getpass(), which means it will be visible on the screen as the user types. This is inconsistent with line 132 where getpass is correctly used for the ticketing instance token. For consistency and security, use getpass.getpass() here as well.
| api_key = input("> ") | |
| api_key = getpass.getpass("> ") |
| def validate_connection(self) -> bool: | ||
| url = f"{self.base_url}/api/v4/user" | ||
| try: | ||
| r = requests.get(url, headers=self._headers(), timeout=10, verify=False) |
There was a problem hiding this comment.
Security concern: SSL verification is disabled with verify=False. This is inconsistent with the disabled warnings in the constructor and makes the application vulnerable to man-in-the-middle attacks.
| params = {"state": "all", "source_branch": source_branch} | ||
| try: | ||
| r = requests.get( | ||
| url, headers=self._headers(), params=params, timeout=10, verify=False) |
There was a problem hiding this comment.
Security concern: SSL verification is disabled with verify=False. This is inconsistent with the disabled warnings in the constructor and makes the application vulnerable to man-in-the-middle attacks.
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
a7a609a to
d5d1d6d
Compare
BenjiMilan
force-pushed
the
feature/BWDO-520_review
branch
from
d5d1d6d to
f823495
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def validate_connection(self) -> bool: | ||
| url = f"{self.base_url}/api/v4/user" | ||
| try: | ||
| r = requests.get(url, headers=self._headers(), timeout=10, verify=False) |
There was a problem hiding this comment.
SSL certificate verification is disabled by setting verify=False. This creates a security vulnerability by making the connection susceptible to man-in-the-middle attacks. Consider making SSL verification configurable through a constructor parameter, defaulting to True, similar to how RedmineInstance handles verify_ssl.
| def _prompt_ticket_selection(self) -> tuple[TicketingInstance, str]: | ||
| """Prompt the user to select a ticketing instance and enter a ticket number. | ||
| Raises: | ||
| TicketIdentifierNotFound: If the instance identifier doesn't match any | ||
| in the config. | ||
| Returns: | ||
| tuple[str, str]: The selected ticketing instance identifier and ticket | ||
| number. |
There was a problem hiding this comment.
The function signature in the docstring states it returns tuple[TicketingInstance, str], but the actual return statement on line 281 returns tuple[str, str] (the selected identifier and ticket number). The docstring should be corrected to match the actual return type.
| try: | ||
| issue = self._instance.issue(ticket_id) | ||
| except JIRAError as e: | ||
| if 'permission' in e.text: |
There was a problem hiding this comment.
The error checking in line 89 uses 'permission' in e.text which could raise an AttributeError if e.text is None. Consider adding a check to ensure e.text exists before performing string operations on it.
| if 'permission' in e.text: | |
| error_text = getattr(e, "text", None) | |
| if error_text and 'permission' in error_text: |
| logger.info("Enter the branch prefix (e.g ABC for feature/ABC-123_ticket): ") | ||
| branch_prefix = input("> ") | ||
| print("") | ||
|
|
There was a problem hiding this comment.
User input for branch_prefix is not validated. An empty string or whitespace-only input would create an invalid configuration entry. Consider adding validation to ensure non-empty input.
| branch_prefix = branch_prefix.strip() | |
| if not branch_prefix: | |
| logger.error("Branch prefix cannot be empty or whitespace-only!") | |
| sys.exit(1) |
| logger.info("Enter the base URL: ") | ||
| base_url = input("> ") | ||
| print("") |
There was a problem hiding this comment.
User input for base_url is not validated. An empty string or whitespace-only input would create an invalid configuration entry. Consider adding validation to ensure non-empty and properly formatted URL input.
| def _get_repo_slug(self, remote_url: str) -> str: | ||
| """Return the repository slug (e.g. "org/repo") from a remote url.""" | ||
| if remote_url.startswith("git@"): | ||
| slug = remote_url.split(":", 1)[1] |
There was a problem hiding this comment.
The split operation on line 188 could raise an IndexError if the remote_url doesn't contain a colon. Consider adding validation or error handling for malformed git@ URLs.
| slug = remote_url.split(":", 1)[1] | |
| parts = remote_url.split(":", 1) | |
| if len(parts) < 2 or not parts[1]: | |
| raise ValueError(f"Malformed git@ remote URL: {remote_url}") | |
| slug = parts[1] |
| pattern = input("> ") | ||
| print("") | ||
| elif provider == "gitlab": | ||
| logger.info( | ||
| "Enter the URL for the gitlab instance (e.g. https://gitlab.com " | ||
| "or https://your-instance.com): ") | ||
| url = input("> ") | ||
| print("") |
There was a problem hiding this comment.
User input for pattern is not validated. An empty string or whitespace-only input would create an invalid configuration entry. Consider adding validation to ensure non-empty input.
| pattern = input("> ") | |
| print("") | |
| elif provider == "gitlab": | |
| logger.info( | |
| "Enter the URL for the gitlab instance (e.g. https://gitlab.com " | |
| "or https://your-instance.com): ") | |
| url = input("> ") | |
| print("") | |
| while True: | |
| pattern = input("> ").strip() | |
| print("") | |
| if pattern: | |
| break | |
| logger.error("Pattern cannot be empty. Please enter a non-empty pattern.") | |
| elif provider == "gitlab": | |
| logger.info( | |
| "Enter the URL for the gitlab instance (e.g. https://gitlab.com " | |
| "or https://your-instance.com): ") | |
| while True: | |
| url = input("> ").strip() | |
| print("") | |
| if url: | |
| break | |
| logger.error("URL cannot be empty. Please enter a non-empty URL.") |
| auth_type=auth_type, | ||
| project_prefix=project_prefix | ||
| ) | ||
|
|
There was a problem hiding this comment.
Sensitive credentials (API tokens and passwords) are stored using ConfigManager which persists them to a YAML file. Consider warning users about the security implications of storing credentials in plaintext, or implementing encryption for sensitive values.
| logger.warning( | |
| "Security notice: the API token/password will be stored in the local " | |
| "review configuration file, typically in plaintext. Ensure that this " | |
| "file is kept secure and not shared with others." | |
| ) |
| git_cfg = GitInstanceCfg(url=url, token=api_key, provider=provider) | ||
| ReviewConfig().write_git_data(pattern, git_cfg) |
There was a problem hiding this comment.
Sensitive credentials (API token) are stored using ConfigManager which persists them to a YAML file. Consider warning users about the security implications of storing credentials in plaintext, or implementing encryption for sensitive values.
| host_identifiers = self._config.get_ticket_host_identifiers() | ||
| for identifier in host_identifiers: | ||
| # Matches the identifier, followed by - or _, followed by a number | ||
| if m := re.search(fr'{identifier}[-_]?(\d+)', branch_name): |
There was a problem hiding this comment.
The regex pattern uses an unescaped identifier from the config. If the identifier contains special regex characters (e.g., '.', '*', '['), this could lead to unexpected matching behavior or errors. Consider escaping the identifier using re.escape() before using it in the pattern.
| if m := re.search(fr'{identifier}[-_]?(\d+)', branch_name): | |
| if m := re.search(rf'{re.escape(identifier)}[-_]?(\d+)', branch_name): |
Sc review