Reliably use modified repodata for get_new_repoclosure #14

Merged

adamwill merged 1 commit from fix-new-repoclosure into main 2026-03-22 04:59:57 +00:00

Conversation 3 Commits 1 Files changed 50 +159 -126

adamwill commented 2026-03-21 06:32:10 +00:00

Owner

Copy link

I noticed that https://openqa.fedoraproject.org/tests/4471016
passed when it should not have: it should have caught that the
new gstreamer1-plugins-good package had a dependency issue. I was
able to reproduce this locally. Some debugging indicated that,
in get_new_repoclosure, the base repository that should have been
modified was not behaving as if it was modified.

I think we've been relying on DNF's cache, here. We only actually
used the modified repodata directly in get_modified_repoclosure.
In get_new_repoclosure we were configuring the repo with the same
name, but with its original unmodified path. I think we expected
that DNF would cache the modified metadata and re-use it. This
obviously worked at some point (because we have had correct
failures in get_new_repoclosure), but it's obviously fragile.

Let's improve it by creating the temporary directory where we
store the modified repodata in main() and passing it to both
get_modified_repoclosure() and get_new_repoclosure(); this way
get_new_repoclosure() can access and use the actual modified
repodata. In my local testing, this fixes the bug and we now
correctly find the dependency error.

Signed-off-by: Adam Williamson awilliam@redhat.com

I noticed that https://openqa.fedoraproject.org/tests/4471016 passed when it should not have: it should have caught that the new gstreamer1-plugins-good package had a dependency issue. I was able to reproduce this locally. Some debugging indicated that, in get_new_repoclosure, the base repository that should have been modified was not behaving as if it was modified. I think we've been relying on DNF's cache, here. We only actually used the modified repodata directly in get_modified_repoclosure. In get_new_repoclosure we were configuring the repo with the same name, but with its original unmodified path. I think we expected that DNF would cache the modified metadata and re-use it. This obviously worked at some point (because we *have* had correct failures in get_new_repoclosure), but it's obviously fragile. Let's improve it by creating the temporary directory where we store the modified repodata in main() and passing it to both get_modified_repoclosure() and get_new_repoclosure(); this way get_new_repoclosure() can access and use the actual modified repodata. In my local testing, this fixes the bug and we now correctly find the dependency error. Signed-off-by: Adam Williamson <awilliam@redhat.com>

adamwill added 1 commit 2026-03-21 06:32:10 +00:00

Reliably use modified repodata for get_new_repoclosure ... c2b6ce2a72

I noticed that https://openqa.fedoraproject.org/tests/4471016
passed when it should not have: it should have caught that the
new gstreamer1-plugins-good package had a dependency issue. I was
able to reproduce this locally. Some debugging indicated that,
in get_new_repoclosure, the base repository that should have been
modified was not behaving as if it was modified.

I think we've been relying on DNF's cache, here. We only actually
used the modified repodata directly in get_modified_repoclosure.
In get_new_repoclosure we were configuring the repo with the same
name, but with its original unmodified path. I think we expected
that DNF would cache the modified metadata and re-use it. This
obviously worked at some point (because we *have* had correct
failures in get_new_repoclosure), but it's obviously fragile.

Let's improve it by creating the temporary directory where we
store the modified repodata in main() and passing it to both
get_modified_repoclosure() and get_new_repoclosure(); this way
get_new_repoclosure() can access and use the actual modified
repodata. In my local testing, this fixes the bug and we now
correctly find the dependency error.

Signed-off-by: Adam Williamson <awilliam@redhat.com>

forgejo-actions commented 2026-03-21 06:34:11 +00:00

Copy link

AI Code Review

📋 MR Summary

Fixes a bug where get_new_repoclosure relied on DNF caching to use modified repodata by explicitly sharing the temporary directory containing the modified metadata.

• Key Changes:
  • Extracted the creation of the temporary directory for modified repodata to main().
  • Updated get_modified_repoclosure to accept the shared temporary directory rather than creating its own.
  • Modified get_new_repoclosure to explicitly use the modified repository directory paths via --repofrompath.
• Impact: rmdepcheck.py, tests/test_rmdepcheck.py
• Risk Level: 🟢 Low - The changes mostly involve refactoring where a temporary directory is created and passing it around explicitly to ensure DNF resolves the modified paths, which is structurally safe and localized.

Detailed Code Review

The implementation correctly identifies and fixes the fragility of relying on DNF's cache. By hoisting the creation of the temporary directory up to main(), both get_modified_repoclosure and get_new_repoclosure can reliably access the exact same modified repodata on disk. The refactoring is clean and correctly updates the respective CLI arguments for DNF.

One minor concern regarding the unit tests: in test_get_new_repoclosure, an empty temporary directory is passed. Since get_modified_repoclosure isn't called prior to populate it, DNF might receive a path to a directory that lacks actual repodata, which could lead to unexpected behavior if DNF strictly validates the --repofrompath location. Ensure the test environment properly handles or mocks this scenario.

📂 File Reviews

📄 `tests/test_rmdepcheck.py` - Updates test functions to pass a temporary directory to the modified functions.

• Minor [Testing]: In test_get_new_repoclosure, tempdir is empty because get_modified_repoclosure is never called to populate it. Passing an unpopulated directory to get_new_repoclosure might cause DNF to fail or emit warnings if it attempts to resolve --repofrompath and cannot find a repodata subdirectory.
  • Suggestion: If DNF actually runs in these tests (and isn't mocked), you may need to either mock the DNF call or ensure that the required repodata structure exists in the tempdir before calling get_new_repoclosure.

✅ Summary

• Overall Assessment: The logic perfectly matches the intended fix, reliably forcing DNF to use the modified metadata path. No critical issues were found.
• Minor Suggestions:
  • Verify that test_get_new_repoclosure successfully executes in CI without DNF failing due to missing repodata inside the empty tempdir.

---

🤖 AI Code Review | Generated with ai-code-review
Platform: Forgejo | AI Provider: gemini | Model: gemini-3-pro-preview

## AI Code Review ### 📋 MR Summary Fixes a bug where `get_new_repoclosure` relied on DNF caching to use modified repodata by explicitly sharing the temporary directory containing the modified metadata. - **Key Changes:** - Extracted the creation of the temporary directory for modified repodata to `main()`. - Updated `get_modified_repoclosure` to accept the shared temporary directory rather than creating its own. - Modified `get_new_repoclosure` to explicitly use the modified repository directory paths via `--repofrompath`. - **Impact:** rmdepcheck.py, tests/test_rmdepcheck.py - **Risk Level:** 🟢 Low - The changes mostly involve refactoring where a temporary directory is created and passing it around explicitly to ensure DNF resolves the modified paths, which is structurally safe and localized. ### Detailed Code Review The implementation correctly identifies and fixes the fragility of relying on DNF's cache. By hoisting the creation of the temporary directory up to `main()`, both `get_modified_repoclosure` and `get_new_repoclosure` can reliably access the exact same modified repodata on disk. The refactoring is clean and correctly updates the respective CLI arguments for DNF. One minor concern regarding the unit tests: in `test_get_new_repoclosure`, an empty temporary directory is passed. Since `get_modified_repoclosure` isn't called prior to populate it, DNF might receive a path to a directory that lacks actual `repodata`, which could lead to unexpected behavior if DNF strictly validates the `--repofrompath` location. Ensure the test environment properly handles or mocks this scenario. #### 📂 File Reviews <details> <summary><strong>📄 `tests/test_rmdepcheck.py`</strong> - Updates test functions to pass a temporary directory to the modified functions.</summary> - **Minor** [Testing]: In `test_get_new_repoclosure`, `tempdir` is empty because `get_modified_repoclosure` is never called to populate it. Passing an unpopulated directory to `get_new_repoclosure` might cause DNF to fail or emit warnings if it attempts to resolve `--repofrompath` and cannot find a `repodata` subdirectory. - *Suggestion:* If DNF actually runs in these tests (and isn't mocked), you may need to either mock the DNF call or ensure that the required repodata structure exists in the `tempdir` before calling `get_new_repoclosure`. </details> ### ✅ Summary - **Overall Assessment:** The logic perfectly matches the intended fix, reliably forcing DNF to use the modified metadata path. No critical issues were found. - **Minor Suggestions:** - Verify that `test_get_new_repoclosure` successfully executes in CI without DNF failing due to missing `repodata` inside the empty `tempdir`. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) **Platform:** Forgejo | **AI Provider:** gemini | **Model:** gemini-3-pro-preview

adamwill force-pushed fix-new-repoclosure from c2b6ce2a72 to 373b91865d 2026-03-21 06:40:20 +00:00 Compare

adamwill commented 2026-03-21 06:41:04 +00:00

Author

Owner

Copy link

Tweaked. I should probably make test_get_new_repoclosure actually test this case properly, too, though.

Tweaked. I should probably make `test_get_new_repoclosure` actually test this case properly, too, though.

adamwill force-pushed fix-new-repoclosure from 373b91865d to 30c5b6a8f7 2026-03-22 04:39:13 +00:00 Compare

adamwill force-pushed fix-new-repoclosure from 30c5b6a8f7 to 4f8278c938 2026-03-22 04:44:24 +00:00 Compare

adamwill force-pushed fix-new-repoclosure from 4f8278c938 to 3e75b7f184 2026-03-22 04:47:31 +00:00 Compare

adamwill force-pushed fix-new-repoclosure from 3e75b7f184 to b8b5612f50 2026-03-22 04:54:20 +00:00 Compare

forgejo-actions commented 2026-03-22 04:57:23 +00:00

Copy link

AI Code Review

📋 MR Summary

This MR fixes an issue where get_new_repoclosure was not using modified repodata by explicitly creating a temporary directory for the modified repodata in main() and passing its path to both get_modified_repoclosure and get_new_repoclosure.

• Key Changes:
  • Removed the creation of a temporary directory from inside get_modified_repoclosure and moved it up to the main() function.
  • Updated get_modified_repoclosure to accept the mreposdir argument and use it for storing modified repository metadata.
  • Modified get_new_repoclosure to accept the mrepos and mreposdir arguments and append them to the dnf repoclosure command parameters.
  • Updated test suite with new mock repository configurations and new expected test outputs.
• Impact: rmdepcheck.py, tests/test_rmdepcheck.py, tests/testdata/*
• Risk Level: 🟡 Medium - Modifies core dependency resolution parameters by passing a new shared directory for modified repodata. If the directory cleanup is mishandled or paths mismatch, it may lead to false positives or IO errors. However, using the standard tempfile.TemporaryDirectory with a context manager minimizes file descriptor and resource leaks.

Detailed Code Review

The structural change of lifting the temporary directory creation to main() is sensible. It establishes a clear, shared source of truth (the modified repodata) that prevents relying on unpredictable DNF caching behavior. The unit tests are properly updated to account for the new function signatures, and a new test package (222) ensures the new logic explicitly fails as intended when testing removed baseline dependencies. The use of a context manager (with tempfile.TemporaryDirectory() as mreposdir:) ensures appropriate teardown. The changes appear correct and align with the described intent.

✅ Summary

• Overall Assessment: The changes directly address the DNF caching fragility by explicitly sharing the modified metadata. The logic appears sound, and test coverage is comprehensive. No critical issues identified.
• Minor Suggestions:
  • Verify that DNF cache expiration won't unexpectedly overwrite the explicitly supplied repository paths in extreme edge cases, although --repofrompath generally overrides default configurations correctly.

---

🤖 AI Code Review | Generated with ai-code-review
Platform: Forgejo | AI Provider: gemini | Model: gemini-3-pro-preview

## AI Code Review ### 📋 MR Summary This MR fixes an issue where `get_new_repoclosure` was not using modified repodata by explicitly creating a temporary directory for the modified repodata in `main()` and passing its path to both `get_modified_repoclosure` and `get_new_repoclosure`. - **Key Changes:** - Removed the creation of a temporary directory from inside `get_modified_repoclosure` and moved it up to the `main()` function. - Updated `get_modified_repoclosure` to accept the `mreposdir` argument and use it for storing modified repository metadata. - Modified `get_new_repoclosure` to accept the `mrepos` and `mreposdir` arguments and append them to the `dnf repoclosure` command parameters. - Updated test suite with new mock repository configurations and new expected test outputs. - **Impact:** rmdepcheck.py, tests/test_rmdepcheck.py, tests/testdata/* - **Risk Level:** 🟡 Medium - Modifies core dependency resolution parameters by passing a new shared directory for modified repodata. If the directory cleanup is mishandled or paths mismatch, it may lead to false positives or IO errors. However, using the standard `tempfile.TemporaryDirectory` with a context manager minimizes file descriptor and resource leaks. ### Detailed Code Review The structural change of lifting the temporary directory creation to `main()` is sensible. It establishes a clear, shared source of truth (the modified repodata) that prevents relying on unpredictable DNF caching behavior. The unit tests are properly updated to account for the new function signatures, and a new test package (`222`) ensures the new logic explicitly fails as intended when testing removed baseline dependencies. The use of a context manager (`with tempfile.TemporaryDirectory() as mreposdir:`) ensures appropriate teardown. The changes appear correct and align with the described intent. ### ✅ Summary - **Overall Assessment:** The changes directly address the DNF caching fragility by explicitly sharing the modified metadata. The logic appears sound, and test coverage is comprehensive. No critical issues identified. - **Minor Suggestions:** - Verify that DNF cache expiration won't unexpectedly overwrite the explicitly supplied repository paths in extreme edge cases, although `--repofrompath` generally overrides default configurations correctly. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) **Platform:** Forgejo | **AI Provider:** gemini | **Model:** gemini-3-pro-preview

adamwill merged commit b8b5612f50 into main 2026-03-22 04:59:57 +00:00

adamwill deleted branch fix-new-repoclosure 2026-03-22 04:59:58 +00:00

adamwill referenced this pull request from quality/paperwork 2026-03-22 14:33:17 +00:00

openQA failure monitoring 2026 sprint 5 #13

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

  

ai-review-please

Request AI review for a pull request

  

Backlog Status

Needs Review

Work items that have been created or updated but need review, refinement, or approval before they can be considered ready for sprint planning.

  

Backlog Status

Ready

Work items that have been reviewed, refined, and are ready to be committed to a sprint. These items have clear requirements and can be worked on immediately when capacity is available.

  

chore

Work that needs to be done but doesn't add new functionality or fix bugs. Maintenance tasks that keep the system running smoothly and the codebase clean.

  

documentation

Work focused on creating, updating, or improving documentation for users, developers, or system administrators.

  

points

01

A task that is simple, small, and has very little uncertainty. It should be easy enough for any team member to pick up and complete quickly, without a lot of discussion or research.

  

points

02

This is a task that is slightly more complex or larger than a "1." It might involve two or three simple steps, but the outcome is still very clear and there's little uncertainty.

  

points

03

Medium-sized task with some noticeable complexity. It might require a bit more thought or collaboration. The steps are well-defined, but it's not a trivial change.

  

points

05

A significant piece of work. It has more complexity and a higher degree of uncertainty than smaller tasks. It might require some research, a design discussion, or collaboration across multiple teams.

  

points

08

Complex task with a lot of uncertainty. It's often the largest size that a team will consider for a single sprint. It likely requires a detailed breakdown and multiple team members to complete.

  

points

13

Any task that feels like it's a "13" or more is considered an Epic. These are too large and uncertain to be worked on in a single sprint and should be broken down.

  

Priority

Critical

This is for work that is urgent and of critical importance and so and must be addressed immediately with a higher priority than all other work, even if not originally planned for the current sprint.

  

Priority

High

This is for work that is critical and needs to be addressed in the next sprint. It's often for urgent bugs, important features, or a major piece of technical debt that is blocking other work.

  

Priority

Low

This is for work that is good to do but not urgent. It's a candidate for being worked on when there's extra time or when it rises in importance. It can stay in the backlog for a longer period.

  

Priority

Medium

This is for important work that should be done soon, but isn't as urgent as a high-priority item. It will likely be considered for the next 1-2 sprints.

  

Sprint Status

Blocked

Work items that cannot progress due to external dependencies, blockers, or issues that need resolution.

  

Sprint Status

Done

Work items that have been completed, reviewed, and integrated. No further work is needed within the current sprint.

  

Sprint Status

In Progress

Work items currently being worked on by a team member. Active development, research, or implementation is happening.

  

Sprint Status

Review

Work items that are complete but need review, testing, or approval before being marked as done.

  

Sprint Status

To Do

Work items that are committed to the sprint but haven't been started yet. These items are ready to be picked up by team members.

  

Technical Debt

Work that addresses shortcuts, workarounds, or suboptimal implementations that were done previously to meet deadlines. These items improve code quality, maintainability, or performance without adding new features.

  

Work Item

Bug

Issues where the system is not working as intended or designed. Bugs represent deviations from expected behavior.

  

Work Item

Epic

Large initiatives that are broken down into multiple smaller work items (user stories, tasks, bugs). Epics represent significant features or projects that span multiple sprints.

  

Work Item

Spike

Time-boxed research, investigation, or proof-of-concept work to reduce uncertainty or explore solutions. Spikes help inform future development decisions.

  

Work Item

Task

Used to break down larger user stories into smaller, manageable implementation pieces. They focus on the specific technical steps needed to deliver the user story's value.

  

Work Item

User Story

User-facing features or functionality that can be framed from a user perspective, describing what someone wants to accomplish and why.

No labels

ai-review-please

Backlog Status

Needs Review

Backlog Status

Ready

chore

documentation

points

01

points

02

points

03

points

05

points

08

points

13

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Sprint Status

Blocked

Sprint Status

Done

Sprint Status

In Progress

Sprint Status

Review

Sprint Status

To Do

Technical Debt

Work Item

Bug

Work Item

Epic

Work Item

Spike

Work Item

Task

Work Item

User Story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

quality/rmdepcheck!14

No description provided.