Repository navigation
Conversation
PR Summary by QodoManage vendor-change policies for Copr repositories
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| save(); | ||
| return true; |
There was a problem hiding this comment.
🟡 Remediation Recommended
2. Failed repo writes still create policies 🐞 Bug ≡ Correctness
save_interactive() returns true after save() without checking whether the repo-file stream opened or completed its writes. If an existing repo file cannot be opened for writing but its permissions can still be changed, the new caller proceeds to create a policy for the in-memory enabled repositories even though the requested repo configuration was not saved.
Agent Prompt
## Issue description
A failed repo-file write can be reported as a successful save, triggering vendor-policy creation.
## Fix Focus Areas
- dnf5-plugins/copr_plugin/copr_repo.cpp[377-396]
- dnf5-plugins/copr_plugin/copr_repo.cpp[461-464]
## Recommended Fix
Check the output stream after opening, writing, and closing the repo file, and propagate any failure instead of returning true. Only invoke policy creation after a verified save.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in a new commit: save() now uses libdnf5::utils::fs::File, which throws libdnf5::FileSystemError on failure.
|
|
||
| choice = 1; | ||
| while (true) { | ||
| std::cerr << _("Enter your choice [1-4, default 1]: "); |
There was a problem hiding this comment.
Don't we have libdnf5::Message for a dialogue with a user?
There was a problem hiding this comment.
I haven't thought about that yet. This is a plugin for the dnf5 application, not for the libdnf5 library. So for now, I'm sticking to the console output style already used in the plugin, and I'll see how things go in the future.
c63afed to
9947690
Compare
The 'enable' command now asks whether a vendor change to/from packages of the repositories it just enabled (the main one, plus any enabled multilib/dependency repositories) should be allowed, and writes the answer as vendor change policies. The default is to not allow it in either direction; with --assumeyes, this default is used without asking. Allowing both directions requires two separate policy files. The two files are distinguished by an "i"/"o" prefix on the base filename. The 'disable' command resets both policies to empty, since disabling a Copr repository disables all of its repo parts and the policies would have no effect anyway. The 'remove' command removes both policy files entirely, skipping ones that don't exist or are distribution-provided (not manageable). All three commands derive the policies' base filename from the repo configuration filename, plus the "i"/"o" prefix and minus the ".repo" extension, so a policy can always be matched back to its Copr repository. CI test updated
259f8a8 to
f6138a7
Compare
The new "--vendor-change=to-repo|from-repo|true|false" option sets the vendor change choice directly instead of asking interactively or using the configured/--assumeyes default. - "to-repo" allows packages from the repositories enabled by this command to replace an already installed package from a different vendor, - "from-repo" allows a package installed from these repositories to be replaced by a package from a different vendor, - "true" allows both - "false" allows neither. Any OptionBool-recognized value can be used in place of "true"/"false". The choice is also configurable as a default, via a "vendor_change" key in the "[main]" section of the Copr configuration, using the same four values. This default is used directly with --assumeyes, or as the interactive prompt's default otherwise; it falls back to "false" if not set. Bump copr plugin version to 0.2.0.
CoprRepo::save() wrote the .repo file via a plain std::ofstream, which does not throw on error unless exceptions() is explicitly enabled -- a failure to open or write the file (e.g. when a directory with the same name exists) would be silently ignored instead of being reported. Use libdnf5::utils::fs::File instead, which raises libdnf5::FileSystemError on filesystem errors.
f6138a7 to
a051e2b
Compare
|
Sorry that I can not provide a formal review (time constraints), but I like the UI (the question regarding Vendors is very tough, so users will be confused for sure -- but I fail to find better or easier wording). For |
Resolves: #3003
The
enablecommand now asks whether packages from the Copr repositories it just enabled (the main one, plus any enabled multilib/dependency repositories) should be allowed to cross a vendor boundary during package replacement (upgrade/downgrade/reinstall), and saves the answer as vendor change policies.Interactive prompt with 4 choices:
to-repo),from-repo),The default is configurable, via a
vendor_changekey in the[main]section of the Copr configuration, using the same four values as new--vendor-change=option. This default is used directly with--assumeyes, or as the interactive prompt's default otherwise; it falls back tofalseif not set.--assumeyesskips the prompt and keeps the default without asking.New
--vendor-change=to-repo|from-repo|true|falseoption sets the choice directly, bypassing both the interactive prompt and the--assumeyesdefault. AnyOptionBool-recognized value works in place oftrue/false.Allowing both directions requires two separate policy files. The two files are distinguished by an "i"/"o" prefix on the base filename.
The
disablecommand resets both policies to empty, since disabling a Copr repository disables all of its repo parts and the policies would have no effect anyway.The
removecommand removes both policy files entirely, skipping ones that don't exist or are distribution-provided (not manageable).All three commands derive the policies' base filename from the repo configuration filename, plus the "i"/"o" prefix and minus the ".repo" extension, so a policy can always be matched back to its Copr repository.
Example outputs:
CI tests are included