Revert "DNF4 to DNF5" - #130
Conversation
📝 WalkthroughWalkthroughThe PR replaces DNF5 packages with ChangesRuntime dependency and cleanup
Configuration loading
Historic data
Page generation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| return historic_data | ||
|
|
||
| log(" Done!") |
| "arch": arch, | ||
| } | ||
| page_name = f"view--{view_conf_id}--{arch}" | ||
| page_name = "view--{view_conf_id}--{arch}".format( |
| json_data = self.load_data(os.path.join(directory, json_file)) | ||
| except Exception as err: | ||
| raise ConfigError(f"Error loading a JSON data file '{json_file}': {err}") from err | ||
| except: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
Dockerfile (1)
3-4: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPreserve the minimal-install settings unless the larger image is intentional.
Lines 3-4 remove
tsflags=nodocsandinstall_weak_deps=False. DNF documents weak dependencies asRecommendsorSupplements, andnodocsas the RPM no-documentation flag. Without these controls, the image can include more packages and files than the runtime needs. (dnf.readthedocs.io)If the image must remain minimal, restore the settings and compare the image size and package list before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 3 - 4, Restore the minimal-install controls on the DNF update and install commands in the Dockerfile by reapplying tsflags=nodocs and install_weak_deps=False. Keep the existing package installation unchanged and verify the resulting image size and package list remain minimal.content_resolver/config_manager.py (1)
864-866: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the YAML parser cause explicitly.
This handler catches
yaml.YAMLError as errbut raisesConfigErrorwithoutfrom err. Raise the wrapper from the caught exception so error reports retain an explicit cause.Proposed fix
- raise ConfigError("Error loading a config '{filename}': {err}".format( - filename=yml_file, - err=err)) + raise ConfigError("Error loading a config '{filename}': {err}".format( + filename=yml_file, + err=err)) from err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@content_resolver/config_manager.py` around lines 864 - 866, Update the YAML error handler around the ConfigError raise to explicitly chain the caught yaml.YAMLError as the cause using the existing err variable, while preserving the current message and yml_file context.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@content_resolver/config_manager.py`:
- Around line 123-127: Update the KeyError handler in _load_config_repo_v2 to
format ConfigError with the available document_id value instead of the undefined
yml_file, preserving the existing missing-baseurl error message behavior.
- Around line 973-976: Update the exception handler around self.load_data(...)
to bind the caught error and replace the bare except with the appropriate
specific exceptions, or except Exception as err if ordinary failures must be
covered. Preserve the ConfigError message while chaining the original exception
so the JSON file failure is reported without raising NameError.
In `@content_resolver/historia_data.py`:
- Around line 132-135: In _read_historic_data, move the “Done!” and blank-line
log calls before return historic_data so both messages execute while preserving
the returned historic_data value.
In `@content_resolver/page_generation.py`:
- Around line 27-29: Update filename construction in the page-generation flow to
reject page_name values containing path separators or traversal components, and
verify the resolved output path remains within output before writing. Preserve
valid filename generation while preventing nested-path failures and writes
outside the output directory; use the existing basename containment approach if
that is the established safeguard.
In `@README.md`:
- Line 3: Update the product description sentence in README.md to use the plural
form “RPM-based Linux distributions” so it agrees with “package sets.”
---
Nitpick comments:
In `@content_resolver/config_manager.py`:
- Around line 864-866: Update the YAML error handler around the ConfigError
raise to explicitly chain the caught yaml.YAMLError as the cause using the
existing err variable, while preserving the current message and yml_file
context.
In `@Dockerfile`:
- Around line 3-4: Restore the minimal-install controls on the DNF update and
install commands in the Dockerfile by reapplying tsflags=nodocs and
install_weak_deps=False. Keep the existing package installation unchanged and
verify the resulting image size and package list remain minimal.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd96de62-ed8a-4b42-8653-3d3cfab0b926
📒 Files selected for processing (9)
.github/workflows/docker-image.yml.github/workflows/fork-only-workflow.ymlDockerfileREADME.mdcontent_resolver/analyzer.pycontent_resolver/config_manager.pycontent_resolver/historia_data.pycontent_resolver/page_generation.pycontent_resolver/utils.py
| except KeyError: | ||
| # FIXME: No reference for `yaml_file` | ||
| raise ConfigError("'{file}.yaml' - is invalid. Repo {id} doesn't list baseurl.".format( | ||
| file=yml_file, | ||
| id=id)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)content_resolver/config_manager\.py$' || true
echo "== context around lines 100-140 =="
if [ -f content_resolver/config_manager.py ]; then
sed -n '80,140p' content_resolver/config_manager.py | nl -ba -v80
fi
echo "== search yml_file/document_id/baseurl in file =="
rg -n "yml_file|document_id|baseurl|_load_config_repo_v2|baseurl" content_resolver/config_manager.py
echo "== outline nearby symbols =="
ast-grep outline content_resolver/config_manager.py --match _load_config_repo_v2 --view expanded || trueRepository: fedora-eln/content-resolver
Length of output: 293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== context around lines 100-140 =="
sed -n '80,140p' content_resolver/config_manager.py | cat -n
echo "== search yml_file/document_id/baseurl in file =="
rg -n "yml_file|document_id|baseurl|_load_config_repo_v2|baseurl" content_resolver/config_manager.py || true
echo "== outline nearby symbols =="
ast-grep outline content_resolver/config_manager.py --match _load_config_repo_v2 --view expanded || trueRepository: fedora-eln/content-resolver
Length of output: 9215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("content_resolver/config_manager.py")
tree = ast.parse(path.read_text())
func = None
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.name == "_load_config_repo_v2":
func = node
break
if func is None:
raise SystemExit("function not found")
print("function_start_line", func.lineno)
scope_names = {target.arg for target in func.args.args}
print("function_args", scope_names)
for node in ast.walk(func):
if isinstance(node, ast.Name) and node.id == "yml_file":
print("yml_file_ref", node.lineno, "context:", ast.unparse(node.slice).replace("'", "'")) if isinstance(node.ctx, ast.Subscript) else print("yml_file_ref", node.lineno)
print("yml_file_context", ast.unparse(node).upper())
PYRepository: fedora-eln/content-resolver
Length of output: 293
Fix the missing baseurl field reference with the available filename.
_load_config_repo_v2 receives document_id, but the KeyError handler formats yml_file, which is not defined. If repo_data["baseurl"] is missing, this raises NameError before returning a load error.
Proposed fix
- except KeyError:
+ except KeyError as err:
# FIXME: No reference for `yaml_file`
- raise ConfigError("'{file}.yaml' - is invalid. Repo {id} doesn't list baseurl.".format(
- file=yml_file,
- id=id))
+ raise ConfigError("'{file}.yaml' - is invalid. Repo {id} doesn't list baseurl.".format(
+ file=document_id,
+ id=id)) from err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except KeyError: | |
| # FIXME: No reference for `yaml_file` | |
| raise ConfigError("'{file}.yaml' - is invalid. Repo {id} doesn't list baseurl.".format( | |
| file=yml_file, | |
| id=id)) | |
| except KeyError as err: | |
| # FIXME: No reference for `yaml_file` | |
| raise ConfigError("'{file}.yaml' - is invalid. Repo {id} doesn't list baseurl.".format( | |
| file=document_id, | |
| id=id)) from err |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 125-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[error] 126-126: Undefined name yml_file
(F821)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@content_resolver/config_manager.py` around lines 123 - 127, Update the
KeyError handler in _load_config_repo_v2 to format ConfigError with the
available document_id value instead of the undefined yml_file, preserving the
existing missing-baseurl error message behavior.
Source: Linters/SAST tools
| except: | ||
| raise ConfigError("Error loading a JSON data file '{filename}': {err}".format( | ||
| filename=json_file, | ||
| err=err)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)content_resolver/config_manager\.py$|config_manager\.py$' || true
echo
echo "Relevant source around lines 950-985:"
if [ -f content_resolver/config_manager.py ]; then
sed -n '940,985p' content_resolver/config_manager.py | nl -ba -v940
fi
echo
echo "Exception handlers with err reference in file:"
rg -n "except|err|ConfigError|load_data|json_file|%s|\\{err\\}" content_resolver/config_manager.py || true
echo
echo "Python semantic probe for bare except and undefined variable in except handler:"
python3 - <<'PY'
def load_raises():
raise ValueError("original failure")
try:
try:
load_raises()
except:
raise Exception("wrapper failed: {err}".format(err=err))
except Exception as e:
print("caught:", type(e).__name__)
print("args:", args := e.args)
print("cause:", type(e.__cause__).__name__ if e.__cause__ is not None else None)
PYRepository: fedora-eln/content-resolver
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)content_resolver/config_manager\.py$|config_manager\.py$' || true
echo
echo "Relevant source around lines 950-985:"
if [ -f content_resolver/config_manager.py ]; then
sed -n '940,985p' content_resolver/config_manager.py | nl -ba -v940
fi
echo
echo "Exception handlers with err reference in file:"
rg -n "except|err|ConfigError|load_data|json_file|%s|\\{err\\}" content_resolver/config_manager.py || true
echo
echo "Python semantic probe for bare except and undefined variable in except handler:"
python3 - <<'PY'
def load_raises():
raise ValueError("original failure")
try:
try:
load_raises()
except:
raise Exception("wrapper failed: {err}".format(err=err))
except Exception as e:
print("caught:", type(e).__name__)
print("args:", args := e.args)
print("cause:", type(e.__cause__).__name__ if e.__cause__ is not None else None)
PYRepository: fedora-eln/content-resolver
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("content_resolver/config_manager.py")
lines = path.read_text(encoding="utf-8").splitlines()
for start, end in [(940, 990), (1, 50)]:
print(f"\nLines {start}-{end}:")
for i in range(max(1, start), min(len(lines), end) + 1):
print(f"{i:5}: {lines[i-1]}")
PY
echo
echo "Relevant exception handling references:"
python3 - <<'PY'
from pathlib import Path
for i, line in enumerate(Path("content_resolver/config_manager.py").read_text().splitlines(), 1):
if any(tok in line for tok in ["except", "ConfigError", "load_data", "json_file", "{err}", "parse_files", "parse_file"]):
print(f"{i:5}: {line}")
PY
echo
echo "Semantic probe for bare except and undefined except-bound variable:"
python3 - <<'PY'
def load_data():
raise ValueError("original failure")
try:
try:
load_data()
except:
raise BaseException("wrapper failed: {err}".format(err=err))
except BaseException as exc:
print("caught:", type(exc).__name__)
print("exception raises NameError:", type(exc).__name__ == "NameError")
PYRepository: fedora-eln/content-resolver
Length of output: 9163
Bind the JSON load exception and avoid a bare except.
If self.load_data(...) fails, except: catches the failure but does not assign err. Formatting the ConfigError message then raises NameError, so the bad JSON file is not added to the reported errors. Catch the specific exceptions from load_data; if this wrapper must cover ordinary failures, use except Exception as err and chain the exception.
🧰 Tools
🪛 Ruff (0.16.1)
[error] 973-973: Do not use bare except
(E722)
[warning] 974-976: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[error] 976-976: Undefined name err
(F821)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@content_resolver/config_manager.py` around lines 973 - 976, Update the
exception handler around self.load_data(...) to bind the caught error and
replace the bare except with the appropriate specific exceptions, or except
Exception as err if ordinary failures must be covered. Preserve the ConfigError
message while chaining the original exception so the JSON file failure is
reported without raising NameError.
Source: Linters/SAST tools
| return historic_data | ||
|
|
||
| log(" Done!") | ||
| log("") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Move the completion log before the return.
return historic_data on Line 132 exits _read_historic_data. The new log calls on Lines 134-135 never execute. Move both calls before the return.
Proposed fix
- return historic_data
-
log(" Done!")
log("")
+ return historic_data📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return historic_data | |
| log(" Done!") | |
| log("") | |
| log(" Done!") | |
| log("") | |
| return historic_data |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@content_resolver/historia_data.py` around lines 132 - 135, In
_read_historic_data, move the “Done!” and blank-line log calls before return
historic_data so both messages execute while preserving the returned
historic_data value.
| filename = ("{page_name}.html".format( | ||
| page_name=page_name.replace(":", "--") | ||
| )) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve the output-directory boundary.
The new filename keeps path separators from page_name. The previous basename containment was removed. Callers insert metadata into page names, including maintainer, pkg_name, and srpm_name on Lines 274-282 and 478-498. If a value contains / and traversal components, open(os.path.join(output, filename), "w") on Line 34 can write outside output. A slash without traversal can also cause a failure because nested directories are not created. Reject path separators and verify the resolved path stays under output, or restore the basename check.
Proposed fix
- filename = ("{page_name}.html".format(
- page_name=page_name.replace(":", "--")
- ))
+ safe_page_name = page_name.replace(":", "--")
+ if "/" in safe_page_name or "\\" in safe_page_name:
+ raise ValueError("Invalid page name")
+ filename = "{}.html".format(safe_page_name)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filename = ("{page_name}.html".format( | |
| page_name=page_name.replace(":", "--") | |
| )) | |
| safe_page_name = page_name.replace(":", "--") | |
| if "/" in safe_page_name or "\\" in safe_page_name: | |
| raise ValueError("Invalid page name") | |
| filename = "{}.html".format(safe_page_name) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@content_resolver/page_generation.py` around lines 27 - 29, Update filename
construction in the page-generation flow to reject page_name values containing
path separators or traversal components, and verify the resolved output path
remains within output before writing. Preserve valid filename generation while
preventing nested-path failures and writes outside the output directory; use the
existing basename containment approach if that is the established safeguard.
Source: Linters/SAST tools
| [](https://github.com/fedora-eln/content-resolver/actions/workflows/docker-image.yml) | ||
|
|
||
| Content Resolver makes it easy to define and inspect package sets of RPM-based Linux distributions. | ||
| Content Resolver makes it easy to define and inspect package sets of RPM-based Linux distribution. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the product description grammar.
Line 3 uses “package sets of RPM-based Linux distribution” without an article. Use the plural form for consistency with “package sets.”
Proposed wording
-Content Resolver makes it easy to define and inspect package sets of RPM-based Linux distribution.
+Content Resolver makes it easy to define and inspect package sets for RPM-based Linux distributions.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Content Resolver makes it easy to define and inspect package sets of RPM-based Linux distribution. | |
| Content Resolver makes it easy to define and inspect package sets for RPM-based Linux distributions. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 3, Update the product description sentence in README.md to
use the plural form “RPM-based Linux distributions” so it agrees with “package
sets.”
2 participants
Reverts #121
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor