Skip to content

Add buildroot repo to non-dev Copr chroots#347

Merged
nforro merged 1 commit intopackit:mainfrom
nforro:copr
Nov 10, 2025
Merged

Add buildroot repo to non-dev Copr chroots#347
nforro merged 1 commit intopackit:mainfrom
nforro:copr

Conversation

@nforro
Copy link
Copy Markdown
Member

@nforro nforro commented Nov 10, 2025

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds functionality to include a buildroot repository for non-development Copr chroots. The implementation is mostly correct and the tests are updated accordingly. However, I've identified a potential bug where a missing configuration key could lead to runtime failures, and an inefficiency where configuration is loaded multiple times. My review includes suggestions to address these points.

Comment thread mcp_server/copr_tools.py Outdated
Comment thread mcp_server/copr_tools.py Outdated
@nforro
Copy link
Copy Markdown
Member Author

nforro commented Nov 10, 2025

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds functionality to include a buildroot repository for non-development Copr chroots. The changes refactor _branch_to_chroot for better separation of concerns and add logic in build_package to modify the chroot configuration. The tests are updated accordingly. My review identifies a logic error in the test suite regarding an incorrect expectation for function call counts, and a minor suggestion for more specific exception handling in the new code.

Comment thread mcp_server/tests/unit/test_copr.py Outdated
Comment thread mcp_server/copr_tools.py
@nforro
Copy link
Copy Markdown
Member Author

nforro commented Nov 10, 2025

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds functionality to include a buildroot repository for non-dev Copr chroots. The changes are logical and include corresponding test updates. I've identified a couple of areas for improvement:

  • In mcp_server/copr_tools.py, using urljoin for constructing the repository URL would be more robust.
  • In mcp_server/tests/unit/test_copr.py, a hardcoded URL is duplicated, which affects maintainability.
    Overall, the changes are good, and my suggestions are aimed at improving robustness and maintainability.

Comment thread mcp_server/copr_tools.py Outdated
Comment thread mcp_server/tests/unit/test_copr.py Outdated
Comment thread mcp_server/tests/unit/test_copr.py Outdated
@nforro
Copy link
Copy Markdown
Member Author

nforro commented Nov 10, 2025

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds functionality to include a buildroot repository for non-development Copr chroots. The approach of moving the rhel_config loading out of _branch_to_chroot is a good refactoring.

My main feedback concerns the implementation of adding the additional repository, which currently overwrites any existing repositories on the chroot. I've suggested a change to fetch the existing configuration and append the new repository to avoid this potential issue. I've also noted that the corresponding unit test will need to be updated to reflect this corrected logic.

Comment thread mcp_server/copr_tools.py
Comment thread mcp_server/tests/unit/test_copr.py
Copy link
Copy Markdown
Member

@lbarcziova lbarcziova left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

Signed-off-by: Nikola Forró <nforro@redhat.com>
@nforro
Copy link
Copy Markdown
Member Author

nforro commented Nov 10, 2025

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces functionality to add a buildroot repository to non-development Copr chroots. The changes are well-structured, including a helpful refactoring of the _branch_to_chroot function to improve testability by decoupling it from configuration loading. The new logic correctly identifies non-dev chroots and idempotently adds the required repository using ProjectChrootProxy. The accompanying tests have been updated to reflect these changes. I have one suggestion to enhance the test coverage for a specific scenario.

Comment thread mcp_server/tests/unit/test_copr.py
@nforro nforro merged commit b63940e into packit:main Nov 10, 2025
7 checks passed
@nforro nforro deleted the copr branch November 10, 2025 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants