git_utils: Fix behavior for RM request #1126
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR implements support for RM requests by introducing an rm_operators parameter in git_utils.push_configs_to_git, propagating it through the build pipeline, implementing operator directory removal in the repository, and adding/updating unit tests to cover various RM scenarios. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Potential risk of removing unintended directories. (link)
General comments:
- In push_configs_to_git, overloading rm_operators as a truthy flag to select removal vs. addition can be confusing—consider refactoring to an explicit operation mode (e.g., enum or separate add/remove parameters) to clarify the control flow.
- Removal commits still use the generic commit message template without noting which operators were deleted—consider customizing the commit message when rm_operators is provided for clearer audit history.
- The RegexMatcher and removal tests share a lot of boilerplate—consider DRYing them up with pytest parametrization or shared test helpers to reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In push_configs_to_git, overloading rm_operators as a truthy flag to select removal vs. addition can be confusing—consider refactoring to an explicit operation mode (e.g., enum or separate add/remove parameters) to clarify the control flow.
- Removal commits still use the generic commit message template without noting which operators were deleted—consider customizing the commit message when rm_operators is provided for clearer audit history.
- The RegexMatcher and removal tests share a lot of boilerplate—consider DRYing them up with pytest parametrization or shared test helpers to reduce duplication.
## Individual Comments
### Comment 1
<location> `iib/workers/tasks/git_utils.py:94` </location>
<code_context>
+ # Remove operators from the Git repo if an RM operation is requested
+ for operator_package in rm_operators:
+ operator_dir = os.path.join(repo_configs_dir, operator_package)
+ shutil.rmtree(operator_dir)
else:
- shutil.copytree(src_configs_path, repo_configs_dir)
</code_context>
<issue_to_address>
Potential risk of removing unintended directories.
Validate operator_package to prevent path traversal and ensure only intended directories are removed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@nmars @yashvardhannanavati during yesterday's night I was thinking: If we do remove all images in the catalog that are not present in the image we might fall in a situation that we may remove unnecessary operators because we were blindly removing what we thought to be the correct ones. Then an enlightenment came into my mind: why do we pass a boolean saying There might be some adjustments required but I would love to hear your opinion in the matter |
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In push_configs_to_git, consider guarding shutil.rmtree with an exists check or ignore_errors=True (or wrap in try/except) to avoid failures when the operator directory is already gone.
- The RegexMatcher helper in the tests is generic enough to be reused; think about extracting it into a shared test utilities module so other tests can import it instead of redefining it here.
- The conditional logic that switches between rm_operators and operator_packages in push_configs_to_git is a bit hard to follow—consider splitting the add and remove workflows into separate functions or using an explicit flag to make the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In push_configs_to_git, consider guarding shutil.rmtree with an exists check or ignore_errors=True (or wrap in try/except) to avoid failures when the operator directory is already gone.
- The RegexMatcher helper in the tests is generic enough to be reused; think about extracting it into a shared test utilities module so other tests can import it instead of redefining it here.
- The conditional logic that switches between rm_operators and operator_packages in push_configs_to_git is a bit hard to follow—consider splitting the add and remove workflows into separate functions or using an explicit flag to make the intent clearer.
## Individual Comments
### Comment 1
<location> `iib/workers/tasks/git_utils.py:90` </location>
<code_context>
)
- if operator_packages:
+ if rm_operators:
+ # Remove operators from the Git repo if an RM operation is requested
+ for operator_package in set(rm_operators):
+ if operator_package: # We don't want to remove the whole /configs directory
+ operator_dir = os.path.join(repo_configs_dir, operator_package)
+ shutil.rmtree(operator_dir)
+ elif operator_packages:
+ # Add content to the Git repo if an ADD or any other operation is requested
</code_context>
<issue_to_address>
Consider handling missing operator directories gracefully during removal.
Catching FileNotFoundError when removing operator directories will prevent failures if the directory is missing, which may be expected in some cases.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
operator_dir = os.path.join(repo_configs_dir, operator_package)
shutil.rmtree(operator_dir)
=======
operator_dir = os.path.join(repo_configs_dir, operator_package)
try:
shutil.rmtree(operator_dir)
except FileNotFoundError:
# Directory does not exist, nothing to remove
pass
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `iib/workers/tasks/git_utils.py:105` </location>
<code_context>
shutil.copytree(src_pkg_dir, dest_pkg_dir, dirs_exist_ok=True)
else:
+ # Add all content to the Git repo for ADD or any other operation
shutil.copytree(src_configs_path, repo_configs_dir)
# Print git status to the logs
</code_context>
<issue_to_address>
Consider using dirs_exist_ok=True for shutil.copytree to avoid errors if the destination exists.
If repo_configs_dir might already exist, setting dirs_exist_ok=True will prevent errors and improve robustness.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
shutil.copytree(src_configs_path, repo_configs_dir)
=======
shutil.copytree(src_configs_path, repo_configs_dir, dirs_exist_ok=True)
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/test_workers/test_tasks/test_build.py:469` </location>
<code_context>
index_repo_map=url_repo_map,
+ rm_operators=None,
)
mock_pcg.assert_called_once_with(
request_id=1,
from_index=from_index,
src_configs_path='/tmp/catalog_dir',
index_repo_map=url_repo_map,
+ rm_operators=None,
)
else:
</code_context>
<issue_to_address>
Consider adding a test for non-None rm_operators in build tasks.
Current tests only cover rm_operators=None. Please add a test with a non-empty rm_operators list to ensure correct handling of removal requests.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
index_repo_map=url_repo_map,
rm_operators=None,
)
# Verify catalog config handling
from_index=from_index,
src_configs_path='/tmp/catalog_dir',
index_repo_map=url_repo_map,
rm_operators=None,
)
=======
index_repo_map=url_repo_map,
rm_operators=None,
)
# Test with non-empty rm_operators
rm_operators_list = ["operator1", "operator2"]
build_task(
request_id=2,
from_index=from_index,
src_configs_path='/tmp/catalog_dir',
index_repo_map=url_repo_map,
rm_operators=rm_operators_list,
)
mock_pcg.assert_any_call(
request_id=2,
from_index=from_index,
src_configs_path='/tmp/catalog_dir',
index_repo_map=url_repo_map,
rm_operators=rm_operators_list,
)
# Verify catalog config handling
from_index=from_index,
src_configs_path='/tmp/catalog_dir',
index_repo_map=url_repo_map,
rm_operators=None,
)
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JAVGan
force-pushed
the
fix-rm-git
branch
3 times, most recently
from
a093a28 to
e77a3ae
Compare
The `RM` request should cause the `git_utils` to remove the operators from the `git` repository and commit the changes, which was something that didn't happen in the previous versions. This commit changes the `git_utils` module as by adding a new parameter to `push_configs_to_git` named `rm_operators` which should only receive a list of operators when a `RM` operation is handled. It also updates the `build` module to send the `RM` operators list to `push_configs_to_git` by passing it over to the necessary functions. Finally, it adds new unit-tests and improve some existing ones. Signed-off-by: Jonathan Gangi <jgangi@redhat.com> Assisted-by: Cursor/Gemini
yashvardhannanavati
approved these changes
Sep 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
RMrequest should cause thegit_utilsto remove the operators from thegitrepository and commit the changes, which was something that didn't happen in the previous versions.This commit changes the
git_utilsmodule as by adding a new parameter topush_configs_to_gitnamedrm_operatorswhich should only receive a list of operators when aRMoperation is handled.It also updates the
buildmodule to send theRMoperators list topush_configs_to_gitby passing it over to the necessary functions.Finally, it adds new unit-tests and improve some existing ones.
Signed-off-by: Jonathan Gangi jgangi@redhat.com
Assisted-by: Cursor/Gemini
Summary by Sourcery
Add support for handling RM requests by introducing a new rm_operators parameter to push_configs_to_git, implementing removal logic for operators during Git updates, and propagating this parameter through the build workflow. Update and extend unit tests to cover removal scenarios and introduce a RegexMatcher helper for verifying directory deletions.
New Features:
Enhancements:
Tests: