quality/fedora-easy-karma
8
0
Fork 0
Code Issues 4 Pull requests 1 Projects Releases Packages Activity Actions

Refactor: Modularize monolithic codebase into clean architecture (v0.60) #72

Manually merged
kparal merged 14 commits from refactor/easy-karma into master 2026-02-26 10:29:44 +00:00
Conversation 15 Commits 14 Files changed 19 +1641 -1775
lruzicka commented 2025-12-15 09:35:29 +00:00
Owner
Copy link

Refactored the entire application from a single 1065-line monolithic file
into a well-organized modular package structure with clear separation of
concerns. This major architectural improvement maintains 100% functional
compatibility while significantly improving code maintainability.

New modular structure (fedora_easy_karma/):

  • init.py: Package initialization with version 0.60
  • application.py: Main orchestrator coordinating all components
  • bodhi_communicator.py: Bodhi API interactions (auth, queries, CSRF handling)
  • package_manager.py: DNF integration and package operations
  • config_manager.py: Configuration and argument parsing
  • state_manager.py: Persistent state management (cache/ignore files)
  • output_formatter.py: Text formatting and display utilities
  • user_interface.py: User input and terminal interaction
  • constants.py: Centralized constants and usage text

Key improvements:

  • Each class has a single, well-defined responsibility
  • Clear interfaces between components using callback patterns
  • Easier to test individual components
  • Better code organization and reduced duplication
  • Consistent error handling across modules

Additional changes:

  • fedora-easy-karma.py: Replaced with minimal entry point (29 lines)
  • fedora-easy-karma-old.py: Preserved original monolithic code (v0.56)
  • CLAUDE.md: Added comprehensive developer documentation
  • .gitignore: Added to exclude Python cache files

All original features, command-line interface, and user experience remain
unchanged. Configuration files and state files are backward compatible.

Fixes: #69
Co-authored: Claude-2.5

Refactored the entire application from a single 1065-line monolithic file into a well-organized modular package structure with clear separation of concerns. This major architectural improvement maintains 100% functional compatibility while significantly improving code maintainability. New modular structure (fedora_easy_karma/): - __init__.py: Package initialization with version 0.60 - application.py: Main orchestrator coordinating all components - bodhi_communicator.py: Bodhi API interactions (auth, queries, CSRF handling) - package_manager.py: DNF integration and package operations - config_manager.py: Configuration and argument parsing - state_manager.py: Persistent state management (cache/ignore files) - output_formatter.py: Text formatting and display utilities - user_interface.py: User input and terminal interaction - constants.py: Centralized constants and usage text Key improvements: - Each class has a single, well-defined responsibility - Clear interfaces between components using callback patterns - Easier to test individual components - Better code organization and reduced duplication - Consistent error handling across modules Additional changes: - fedora-easy-karma.py: Replaced with minimal entry point (29 lines) - fedora-easy-karma-old.py: Preserved original monolithic code (v0.56) - CLAUDE.md: Added comprehensive developer documentation - .gitignore: Added to exclude Python cache files All original features, command-line interface, and user experience remain unchanged. Configuration files and state files are backward compatible. Fixes: #69 Co-authored: Claude-2.5
Refactored the entire application from a single 1065-line monolithic file
into a well-organized modular package structure with clear separation of
concerns. This major architectural improvement maintains 100% functional
compatibility while significantly improving code maintainability.

New modular structure (fedora_easy_karma/):
- __init__.py: Package initialization with version 0.60
- application.py: Main orchestrator coordinating all components
- bodhi_communicator.py: Bodhi API interactions (auth, queries, CSRF handling)
- package_manager.py: DNF integration and package operations
- config_manager.py: Configuration and argument parsing
- state_manager.py: Persistent state management (cache/ignore files)
- output_formatter.py: Text formatting and display utilities
- user_interface.py: User input and terminal interaction
- constants.py: Centralized constants and usage text

Key improvements:
- Each class has a single, well-defined responsibility
- Clear interfaces between components using callback patterns
- Easier to test individual components
- Better code organization and reduced duplication
- Consistent error handling across modules

Additional changes:
- fedora-easy-karma.py: Replaced with minimal entry point (29 lines)
- fedora-easy-karma-old.py: Preserved original monolithic code (v0.56)
- CLAUDE.md: Added comprehensive developer documentation
- .gitignore: Added to exclude Python cache files

All original features, command-line interface, and user experience remain
unchanged. Configuration files and state files are backward compatible.

Co-authored: Claude-2.5
When errors should propagate to main(), also change the other couple
of files.
lruzicka force-pushed refactor/easy-karma from 5ca3b51ac1 to 1d496f12d6 2026-01-06 12:40:39 +00:00 Compare
lruzicka changed title from (WIP) Refactor: Modularize monolithic codebase into clean architecture (v0.60) to Refactor: Modularize monolithic codebase into clean architecture (v0.60) 2026-01-06 12:43:51 +00:00
kparal commented 2026-01-08 13:37:24 +00:00
Owner
Copy link

Can you please also generate requirements.txt and verify that it works in a clean venv? I'm having issues to populate all the dependencies.

Let's add Fixes: #69 to the commit message.

I would recommend shortening the module names, e.g. typing bodhi_communicator feels excessive when it can be just bodhi.

I'll provide a more detailed review once I can run it in my venv.

Can you please also generate `requirements.txt` and verify that it works in a clean venv? I'm having issues to populate all the dependencies. Let's add `Fixes: #69` to the commit message. I would recommend shortening the module names, e.g. typing `bodhi_communicator` feels excessive when it can be just `bodhi`. I'll provide a more detailed review once I can run it in my venv.
kparal changed title from Refactor: Modularize monolithic codebase into clean architecture (v0.60) to WIP: Refactor: Modularize monolithic codebase into clean architecture (v0.60) 2026-01-08 13:37:36 +00:00
kparal changed title from WIP: Refactor: Modularize monolithic codebase into clean architecture (v0.60) to Refactor: Modularize monolithic codebase into clean architecture (v0.60) 2026-01-08 13:37:55 +00:00
kparal self-assigned this 2026-01-08 13:38:15 +00:00
adamwill commented 2026-01-08 17:19:28 +00:00
Owner
Copy link

I'll try and look at this too, but in general I am suspicious of any 'refactoring' that leaves the project 2x the size it was before.

Clearly fedora-easy-karma-old.py should not exist. This is a git repository. We don't need old versions of things lying around the current state of the repository.

The presence of CLAUDE.md effectively now ties the project to LLM generation; if we did any significant manual changes we would also have to update CLAUDE.md and hope we did it in a way Claude (or any other LLM being used to work on the code base) likes. I'm not sure I love this. This kind of project design documentation is useful in itself for large projects, but should be formatted in a neutral way rather than one tied to a specific LLM. For a project of this size I'm not sure its value outweighs the expense of maintaining it.

I think it'd be much easier to be confident in a refactor if the original implementation had tests, and the refactor passed the test suite with as few modifications as possible. A rewrite of something from 1000 lines to 2000 lines with no tests on either side is an inherently risky operation.

I'll try and look at this too, but in general I am suspicious of any 'refactoring' that leaves the project 2x the size it was before. Clearly fedora-easy-karma-old.py should not exist. This is a git repository. We don't need old versions of things lying around the current state of the repository. The presence of CLAUDE.md effectively now ties the project to LLM generation; if we did any significant manual changes we would also have to update CLAUDE.md and hope we did it in a way Claude (or any other LLM being used to work on the code base) likes. I'm not sure I love this. This kind of project design documentation is useful in itself for large projects, but should be formatted in a neutral way rather than one tied to a specific LLM. For a project of this size I'm not sure its value outweighs the expense of maintaining it. I think it'd be much easier to be confident in a refactor if the original implementation had tests, and the refactor passed the test suite with as few modifications as possible. A rewrite of something from 1000 lines to 2000 lines with no tests on either side is an inherently risky operation.
kparal commented 2026-01-09 11:49:01 +00:00
Owner
Copy link

@adamwill wrote in #72 (comment):

Clearly fedora-easy-karma-old.py should not exist. This is a git repository. We don't need old versions of things lying around the current state of the repository.

I don't mind it for the review, it makes comparisons easier. But yes, we should delete it at the end of review. (Or now, either way works).

The presence of CLAUDE.md effectively now ties the project to LLM generation; if we did any significant manual changes we would also have to update CLAUDE.md and hope we did it in a way Claude (or any other LLM being used to work on the code base) likes. I'm not sure I love this. This kind of project design documentation is useful in itself for large projects, but should be formatted in a neutral way rather than one tied to a specific LLM. For a project of this size I'm not sure its value outweighs the expense of maintaining it.

Right. It's probably useful for the duration of some big feature development, but we probably don't want to commit it. When enough changes accumulate, it would only confuse future AI.

I think it'd be much easier to be confident in a refactor if the original implementation had tests, and the refactor passed the test suite with as few modifications as possible.

That would of course be great, but that's not going to happen. This refactor is a step forward for having some tests. The previous code was a spaghetti monster, writing tests for it would be very hard and would likely necessitate changes exactly as we do now. And the test suite would likely be completely changed during the rewrite. The idea is that we need to split this into reasonable pieces and standalone objects and methods, and only then it makes sense to cover it with tests or do some larger future work.

@adamwill wrote in https://forge.fedoraproject.org/quality/fedora-easy-karma/pulls/72#issuecomment-259625: > Clearly fedora-easy-karma-old.py should not exist. This is a git repository. We don't need old versions of things lying around the current state of the repository. I don't mind it for the review, it makes comparisons easier. But yes, we should delete it at the end of review. (Or now, either way works). > The presence of CLAUDE.md effectively now ties the project to LLM generation; if we did any significant manual changes we would also have to update CLAUDE.md and hope we did it in a way Claude (or any other LLM being used to work on the code base) likes. I'm not sure I love this. This kind of project design documentation is useful in itself for large projects, but should be formatted in a neutral way rather than one tied to a specific LLM. For a project of this size I'm not sure its value outweighs the expense of maintaining it. Right. It's probably useful for the duration of some big feature development, but we probably don't want to commit it. When enough changes accumulate, it would only confuse future AI. > I think it'd be much easier to be confident in a refactor if the original implementation had tests, and the refactor passed the test suite with as few modifications as possible. That would of course be great, but that's not going to happen. This refactor is a step forward for having some tests. The previous code was a spaghetti monster, writing tests for it would be very hard and would likely necessitate changes exactly as we do now. And the test suite would likely be completely changed during the rewrite. The idea is that we need to split this into reasonable pieces and standalone objects and methods, and only then it makes sense to cover it with tests or do some larger future work.
lruzicka commented 2026-01-09 12:55:53 +00:00
Author
Owner
Copy link

@adamwill wrote in #72 (comment):

I would like to provide some explanations:

I adopted Fedora Easy Karma as a learning project because it was not that complicated and it has been in stagnation for a certain period of time. It did not have any issues that would originate from the community users (apart from us), which suggested that most of the users were already satisfied with its functionality. That does not apply any pressure to quick development or critical fixes and leaves time to explore and try out things that I could learn from. The learning process also includes to try out LLMs and see if they are of any use (AI goal and such). Since I dived into the project, I have been able to fix several issues and I believe the project is now in a better state than it was before.

Anything, I have done so far, I have done with best intentions, but it does not have to be "the best" solution ever. I take the project as a process, as a constant development and improvement in terms of Kaizen. I do not dare to say that I am able to hit the bull's eye in the first attempt. I also have not that many experience with programming anything else than one-py-file applications, but I still think that this step makes the application "better than it was before". That's why I decided to invite other people to have their say.

I'll try and look at this too, but in general I am suspicious of any 'refactoring' that leaves the project 2x the size it was before.

I know that you prefer compactness over wordiness, but that could make the code less readable for people who are not that good in reading idioms. Is it better to spend time trying to understand a couple of dense statements, or to spend time (maybe less) to go over more statements which a clear and concise? I still advocate for the second. And just because it is easier for me, but not everybody outthere is as an avid code reader as you are. Also, do not just calculate lines. Many have been introduced because the model added quite a number of new methods (which replaced the bulky one method application), the method info strings for each of them, and added the overall description of how the project works in CLAUDE.md. I also let another model help me with the post refactoring review and where it suggested the code was "dangerous" I try to add checks to make it "less dangerous". which also added a couple of extra lines. I wonder, if I deleted the old py file, would the number of the lines drop in the commit overview?

Clearly fedora-easy-karma-old.py should not exist. This is a git repository. We don't need old versions of things lying around the current state of the repository.

I plan to delete it before we decide to merge this.

The presence of CLAUDE.md effectively now ties the project to LLM generation; if we did any significant manual changes we would also have to update CLAUDE.md and hope we did it in a way Claude (or any other LLM being used to work on the code base) likes. I'm not sure I love this. This kind of project design documentation is useful in itself for large projects, but should be formatted in a neutral way rather than one tied to a specific LLM. For a project of this size I'm not sure its value outweighs the expense of maintaining it.

The presence of CLAUDE.md is not needed and the file can be deleted, renamed, or turned into a more general RAG file if we decide so. My intention is not to turn this project into an AI generated one, I used the model to speed up the conversion into a new form. I would like to have a clean project that is easy to maintain manually and which is also easy to add new features and scale it up.

The old file is messy, the most of the application's functionality was kept in an init function, which (even as a baby pythonista) I see problematic, if not entirely ugly. It was very complicated to add new functions and nearly impossible to turn the output into something better than just a plain CLI.

I think it'd be much easier to be confident in a refactor if the original implementation had tests, and the refactor passed the test suite with as few modifications as possible. A rewrite of something from 1000 lines to 2000 lines with no tests on either side is an inherently risky operation.

I agree. I do not have much experience with writing application tests and my precedesors did not seem to have the need to write them either. I plan to have a test suite later. This new form, IMHO, could offer better fundaments for writing tests than before.

So, I (as a baby pythonista) believe that this PR is an improvement to be worth doing for the future (although not perfect or final). If it is, let's make sure it does not break anything, use it, create new issues to improve, simplify, or shorten stuff, and I can work on this gradually over time to make it better on a step-by-step basis. Or, if it is totally wrong, then let's discard it and I will have a case study of a failed LLM generation attempt and I can start doing it manually (which I would be happy to - but it takes much more time and maybe does not fit into how we newly organize our work), or try with some new models in the future and maybe they will be able to absolutely nail it on the first try with no need for further changes.

@adamwill wrote in https://forge.fedoraproject.org/quality/fedora-easy-karma/pulls/72#issuecomment-259625: I would like to provide some explanations: I adopted Fedora Easy Karma as a learning project because it was not that complicated and it has been in stagnation for a certain period of time. It did not have any issues that would originate from the community users (apart from us), which suggested that most of the users were already satisfied with its functionality. That does not apply any pressure to quick development or critical fixes and leaves time to explore and try out things that I could learn from. The learning process also includes to try out LLMs and see if they are of any use (AI goal and such). Since I dived into the project, I have been able to fix several issues and I believe the project is now in a better state than it was before. Anything, I have done so far, I have done with best intentions, but it does not have to be "the best" solution ever. I take the project as a process, as a constant development and improvement in terms of Kaizen. I do not dare to say that I am able to hit the bull's eye in the first attempt. I also have not that many experience with programming anything else than one-py-file applications, but I still think that this step makes the application "better than it was before". That's why I decided to invite other people to have their say. > I'll try and look at this too, but in general I am suspicious of any 'refactoring' that leaves the project 2x the size it was before. I know that you prefer compactness over wordiness, but that could make the code less readable for people who are not that good in reading idioms. Is it better to spend time trying to understand a couple of dense statements, or to spend time (maybe less) to go over more statements which a clear and concise? I still advocate for the second. And just because it is easier for me, but not everybody outthere is as an avid code reader as you are. Also, do not just calculate lines. Many have been introduced because the model added quite a number of new methods (which replaced the bulky one method application), the method info strings for each of them, and added the overall description of how the project works in `CLAUDE.md`. I also let another model help me with the post refactoring review and where it suggested the code was "dangerous" I try to add checks to make it "less dangerous". which also added a couple of extra lines. I wonder, if I deleted the old py file, would the number of the lines drop in the commit overview? > Clearly fedora-easy-karma-old.py should not exist. This is a git repository. We don't need old versions of things lying around the current state of the repository. > I plan to delete it before we decide to merge this. > The presence of CLAUDE.md effectively now ties the project to LLM generation; if we did any significant manual changes we would also have to update CLAUDE.md and hope we did it in a way Claude (or any other LLM being used to work on the code base) likes. I'm not sure I love this. This kind of project design documentation is useful in itself for large projects, but should be formatted in a neutral way rather than one tied to a specific LLM. For a project of this size I'm not sure its value outweighs the expense of maintaining it. The presence of CLAUDE.md is not needed and the file can be deleted, renamed, or turned into a more general RAG file if we decide so. My intention is not to turn this project into an AI generated one, I used the model to speed up the conversion into a new form. I would like to have a clean project that is easy to maintain manually and which is also easy to add new features and scale it up. The old file is messy, the most of the application's functionality was kept in an __init__ function, which (even as a baby pythonista) I see problematic, if not entirely ugly. It was very complicated to add new functions and nearly impossible to turn the output into something better than just a plain CLI. > > I think it'd be much easier to be confident in a refactor if the original implementation had tests, and the refactor passed the test suite with as few modifications as possible. A rewrite of something from 1000 lines to 2000 lines with no tests on either side is an inherently risky operation. I agree. I do not have much experience with writing application tests and my precedesors did not seem to have the need to write them either. I plan to have a test suite later. This new form, IMHO, could offer better fundaments for writing tests than before. So, I (as a baby pythonista) believe that this PR is an improvement to be worth doing for the future (although not perfect or final). If it is, let's make sure it does not break anything, use it, create new issues to improve, simplify, or shorten stuff, and I can work on this gradually over time to make it better on a step-by-step basis. Or, if it is totally wrong, then let's discard it and I will have a case study of a failed LLM generation attempt and I can start doing it manually (which I would be happy to - but it takes much more time and maybe does not fit into how we newly organize our work), or try with some new models in the future and maybe they will be able to absolutely nail it on the first try with no need for further changes.
adamwill commented 2026-01-09 18:06:19 +00:00
Owner
Copy link

To be clear, I'm not saying that as a rejection of the change. It's just more of a 'code smell' - something that should be explained at least, and considered. LLMs tend to be wordy and expansive - are all the things it added actually necessary? Stuff like that. If we go through and it turns out that yes, we do need all the new LoC, that's fine. I just want to make sure it's checked.

To be clear, I'm not saying that as a rejection of the change. It's just more of a 'code smell' - something that should be explained at least, and considered. LLMs tend to be wordy and expansive - are all the things it added actually necessary? Stuff like that. If we go through and it turns out that yes, we do need all the new LoC, that's fine. I just want to make sure it's checked.
psklenar commented 2026-01-15 08:45:06 +00:00
Owner
Copy link

I tried this script by AI, which found some traceback:

❯ ./fedora-easy-karma.py --product "karma"
INFO: Logging into Bodhi
INFO: Getting list of installed packages...
INFO: Waiting for oraculum instance to return list of packages in updates-testing...
INFO: Waiting for Bodhi for a list of packages in updates-testing karma43...
WARNING: Error while querying Bodhi: {"status": "error", "errors": [{"location": "querystring", "name": "releases", "description": "Invalid releases specified: karma43"}]}
Traceback (most recent call last):
File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/./fedora-easy-karma.py", line 43, in
raise SystemExit(main())
~~~~^^
File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/./fedora-easy-karma.py", line 29, in main
return app.run()
~~~~~~~^^
File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/fedora_easy_karma/application.py", line 145, in run
testing_updates = self._get_testing_updates(release)
File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/fedora_easy_karma/application.py", line 189, in _get_testing_updates
testing_updates = self.bodhi.query_updates(
release, pending=False,
rows_per_page=self.options.bodhi_request_limit)
File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/fedora_easy_karma/bodhi_communicator.py", line 91, in query_updates
result = self.client.query(**query_args)
File "/usr/lib/python3.14/site-packages/bodhi/client/bindings.py", line 130, in wrapper
result = method(*args, **kwargs)
File "/usr/lib/python3.14/site-packages/bodhi/client/bindings.py", line 482, in query
return self.send_request('updates/', verb='GET', params=kwargs)
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.14/site-packages/bodhi/client/bindings.py", line 239, in send_request
raise BodhiClientException(response.text)
bodhi.client.bindings.BodhiClientException: {"status": "error", "errors": [{"location": "querystring", "name": "releases", "description": "Invalid releases specified: karma43"}]}

I tried this script by AI, which found some traceback: ❯ ./fedora-easy-karma.py --product "karma" INFO: Logging into Bodhi INFO: Getting list of installed packages... INFO: Waiting for oraculum instance to return list of packages in updates-testing... INFO: Waiting for Bodhi for a list of packages in updates-testing karma43... WARNING: Error while querying Bodhi: {"status": "error", "errors": [{"location": "querystring", "name": "releases", "description": "Invalid releases specified: karma43"}]} Traceback (most recent call last): File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/./fedora-easy-karma.py", line 43, in <module> raise SystemExit(main()) ~~~~^^ File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/./fedora-easy-karma.py", line 29, in main return app.run() ~~~~~~~^^ File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/fedora_easy_karma/application.py", line 145, in run testing_updates = self._get_testing_updates(release) File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/fedora_easy_karma/application.py", line 189, in _get_testing_updates testing_updates = self.bodhi.query_updates( release, pending=False, rows_per_page=self.options.bodhi_request_limit) File "/home/psklenar/git/fedora-easy-karma/fedora-easy-karma-FORGE/fedora_easy_karma/bodhi_communicator.py", line 91, in query_updates result = self.client.query(**query_args) File "/usr/lib/python3.14/site-packages/bodhi/client/bindings.py", line 130, in wrapper result = method(*args, **kwargs) File "/usr/lib/python3.14/site-packages/bodhi/client/bindings.py", line 482, in query return self.send_request('updates/', verb='GET', params=kwargs) ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.14/site-packages/bodhi/client/bindings.py", line 239, in send_request raise BodhiClientException(response.text) bodhi.client.bindings.BodhiClientException: {"status": "error", "errors": [{"location": "querystring", "name": "releases", "description": "Invalid releases specified: karma43"}]}
👍 1
psklenar commented 2026-01-15 09:04:03 +00:00
Owner
Copy link

[!] POTENTIAL BUG: Script accepted karma '99' which is outside the -1 to 1 range.

like



 inst. RPMS: conmon-2.2.0-1.fc43.x86_64
               OCI container runtime monitor (Installed 0
               days ago)
Karma? -1/0/1 -> vote, 'i' -> ignore, other -> skip> -99
[!] POTENTIAL BUG: Script accepted karma '99' which is outside the -1 to 1 range. like ``` inst. RPMS: conmon-2.2.0-1.fc43.x86_64 OCI container runtime monitor (Installed 0 days ago) Karma? -1/0/1 -> vote, 'i' -> ignore, other -> skip> -99 ```
👍 1
kparal added this to the Sprint 1 project 2026-01-15 12:24:26 +00:00
lruzicka force-pushed refactor/easy-karma from 23164856ba to 042d32349c 2026-01-27 09:45:21 +00:00 Compare 
Previously, when user entered an incorrect product, the application
crashed. Now, we only allow valid product:
F for Fedora and EL- for EPEL.
lruzicka commented 2026-01-27 10:04:10 +00:00
Author
Owner
Copy link

@psklenar wrote in #72 (comment):

I tried this script by AI, which found some traceback:

❯ ./fedora-easy-karma.py --product "karma"

This has been fixed in #b74fb03792.

@psklenar wrote in https://forge.fedoraproject.org/quality/fedora-easy-karma/pulls/72#issuecomment-261485: > I tried this script by AI, which found some traceback: > > ❯ ./fedora-easy-karma.py --product "karma" > This has been fixed in #b74fb03792.
lruzicka commented 2026-01-27 10:28:45 +00:00
Author
Owner
Copy link

@psklenar wrote in #72 (comment):

[!] POTENTIAL BUG: Script accepted karma '99' which is outside the -1 to 1 range.

like



 inst. RPMS: conmon-2.2.0-1.fc43.x86_64
               OCI container runtime monitor (Installed 0
               days ago)
Karma? -1/0/1 -> vote, 'i' -> ignore, other -> skip> -99

As the UI suggests, typing anything different from -1, 0, 1, or i will skip the voting process. It behaves as the older version of F-E-K. I do not think we need to address this in this PR.

@psklenar wrote in https://forge.fedoraproject.org/quality/fedora-easy-karma/pulls/72#issuecomment-261486: > [!] POTENTIAL BUG: Script accepted karma '99' which is outside the -1 to 1 range. > > like > > ```text > > > inst. RPMS: conmon-2.2.0-1.fc43.x86_64 > OCI container runtime monitor (Installed 0 > days ago) > Karma? -1/0/1 -> vote, 'i' -> ignore, other -> skip> -99 > ``` As the UI suggests, typing anything different from `-1`, `0`, `1`, or `i` will skip the voting process. It behaves as the older version of F-E-K. I do not think we need to address this in this PR.
adamwill modified the project from Sprint 1 to Sprint 2 2026-01-27 17:26:22 +00:00
kparal force-pushed refactor/easy-karma from b74fb03792 to cc16636928 2026-02-10 08:43:27 +00:00 Compare
kparal removed this from the Sprint 2 project 2026-02-10 08:45:27 +00:00
kparal commented 2026-02-10 08:47:06 +00:00
Owner
Copy link

I've added Fixes: #69 to the commit message and moved all labels and sprint planning to #69 instead of here. Do git pull; git reset --hard origin/refactor/easy-karma to receive the changed commits.

I've added `Fixes: #69` to the commit message and moved all labels and sprint planning to #69 instead of here. Do `git pull; git reset --hard origin/refactor/easy-karma` to receive the changed commits.
kparal changed title from Refactor: Modularize monolithic codebase into clean architecture (v0.60) to WIP: Refactor: Modularize monolithic codebase into clean architecture (v0.60) 2026-02-13 16:34:47 +00:00
kparal commented 2026-02-13 16:36:27 +00:00
Owner
Copy link

I've started looking into this, and will add some commits where it makes sense to me. Since this is all generated code, Lukas will hopefully not have any hard feelings about it 😉

I've started looking into this, and will add some commits where it makes sense to me. Since this is all generated code, Lukas will hopefully not have any hard feelings about it 😉
👀 1 
This is no longer useful, it's not a single script anymore.
Drop GPL3 license file, the previous file was licensed under just GPL2+.
kparal commented 2026-02-18 18:06:46 +00:00
Owner
Copy link

I've done only cosmetic fixups so far, but I had the idea to let AI compare functional (not structural) differences between the old and new code. And there are many of them, and some of them contain genuine bugs. The problem is, it is very hard to compare the code, because the code was not just shuffled around into different modules, split into smaller methods, etc. It was very often also rewritten. I believe that was a mistake, because then this is not just "let's make it more readable" exercise, but it's a full validation between the old code and the new one. Everything is changed! What we should've done was to ask the agent to restructure the code, but make absolute minimal functional changes. We could of course dump the existing PR and generate it from scratch, but since I've already invested time into this review, it feels like wasted time to me, so I'm trying to see if we can still push this through.

Anyway, here are a few examples of changes that didn't need to happen, and cause regressions:

1. Bugzilla URL is broken (typo)

Old (fedora-easy-karma-old.py line 810): https://bugzilla.redhat.com/12345 (works via redirect)
New (output.py line 242): https://bugzilla.redhat.com/show_bug-cgi?id=12345 (dash instead of dot)

Old:

                bug_id = "%s%d" % (bugzilla_bug_url, bug_id)

New:

                bug_id = f"{bugzilla_bug_url}show_bug-cgi?id={bug_id}"

The new URL uses show_bug-cgi (hyphen) instead of show_bug.cgi (dot). This will produce invalid Bugzilla URLs.

2. Rawhide detection raises unhandled exception

Old (line 588-592) prints and calls sys.exit(1). New (config.py line 290) raises a bare ValueError.
The entry point (fedora-easy-karma.py) only catches errors.UsageError and KeyboardInterrupt, so this ValueError will propagate as an unhandled exception with a full traceback instead of a clean exit message.

This is not fully relevant to the detected problem, but a nice example of completely needless changes. It even rephrased error strings!
Old:

                        print("'Rawhide' found in %s, aborting, because " \
                              "there is no updates-testing for " \
                              "Rawhide" % release_filename)

New:

                        raise ValueError(f"Rawhide detected in {release_filename}! No testing updates are provided for Rawhide.")

It is very hard to compare stuff when everything changes including strings.

3. send_comment retry count is off by one

Old (line 713): for retry in range(0, self.options.retries + 1) = retries+1 attempts (4 with default retries=3).
New (bodhi.py line 149): while retries: and retries = retries - 1 = retries attempts (3 with default retries=3).
New code makes one fewer attempt than old code with the same --retries value.

This is also a perfect example of a method that could've stayed almost 100% intact, but was rewritten instead.
Old:

    def send_comment(self, update, comment, karma):
        for retry in range(0, self.options.retries + 1):
            try:
                res = self.bc.comment(update["updateid"], comment, karma=karma)
                return (True, res)
            except BodhiClientException as e:
                self.warning("Bodhi Client error: %s" % str(e))
                self.errors = True
                if "csrf" in str(e).lower():
                    self.warning("Possible CSRF token mismatch, trying to obtain a new one...")
                    self.refresh_csrf()
        return (False, 'too many errors')

New:

    def send_comment(self, update, comment, karma, retries=3):
        while retries:
            try:
                res = self.client.comment(update["updateid"], comment, karma=karma)
                return (True, res)
            except BodhiClientException as e:
                self.warning("Bodhi Client error: %s" % str(e))
                if "csrf" in str(e).lower():
                    self.warning("Possible CSRF token mismatch, trying to obtain a new one...")
                    self.refresh_csrf()
            retries = retries - 1

        return (False, 'too many errors')

If it wasn't rewritten, the bug wouldn't have been introduced.

These are just a few examples (I have 27 detected problems to go through, and that's just what my AI found). I'll continue with the review tomorrow. But I'm not sure if fixing all of these is the easiest way forward...

I've done only cosmetic fixups so far, but I had the idea to let AI compare **functional** (not structural) differences between the old and new code. And there are many of them, and some of them contain genuine bugs. The problem is, it is very hard to compare the code, because the code was not just shuffled around into different modules, split into smaller methods, etc. It was very often also **rewritten**. I believe that was a mistake, because then this is not just "let's make it more readable" exercise, but it's a full validation between the old code and the new one. Everything is changed! What we should've done was to ask the agent to restructure the code, but make **absolute minimal functional changes**. We could of course dump the existing PR and generate it from scratch, but since I've already invested time into this review, it feels like wasted time to me, so I'm trying to see if we can still push this through. Anyway, here are a few examples of changes that didn't need to happen, and cause regressions: **1. Bugzilla URL is broken (typo)** Old (fedora-easy-karma-old.py line 810): https://bugzilla.redhat.com/12345 (works via redirect) New (output.py line 242): https://bugzilla.redhat.com/show_bug-cgi?id=12345 (dash instead of dot) Old: ``` bug_id = "%s%d" % (bugzilla_bug_url, bug_id) ``` New: ``` bug_id = f"{bugzilla_bug_url}show_bug-cgi?id={bug_id}" ``` The new URL uses show_bug-cgi (hyphen) instead of show_bug.cgi (dot). This will produce invalid Bugzilla URLs. **2. Rawhide detection raises unhandled exception** Old (line 588-592) prints and calls sys.exit(1). New (config.py line 290) raises a bare ValueError. The entry point (fedora-easy-karma.py) only catches errors.UsageError and KeyboardInterrupt, so this ValueError will propagate as an unhandled exception with a full traceback instead of a clean exit message. This is not fully relevant to the detected problem, but a nice example of completely needless changes. It even rephrased error strings! Old: ``` print("'Rawhide' found in %s, aborting, because " \ "there is no updates-testing for " \ "Rawhide" % release_filename) ``` New: ``` raise ValueError(f"Rawhide detected in {release_filename}! No testing updates are provided for Rawhide.") ``` It is very hard to compare stuff when everything changes including strings. **3. send_comment retry count is off by one** Old (line 713): for retry in range(0, self.options.retries + 1) = retries+1 attempts (4 with default retries=3). New (bodhi.py line 149): while retries: and retries = retries - 1 = retries attempts (3 with default retries=3). New code makes one fewer attempt than old code with the same --retries value. This is also a perfect example of a method that could've stayed almost 100% intact, but was rewritten instead. Old: ``` def send_comment(self, update, comment, karma): for retry in range(0, self.options.retries + 1): try: res = self.bc.comment(update["updateid"], comment, karma=karma) return (True, res) except BodhiClientException as e: self.warning("Bodhi Client error: %s" % str(e)) self.errors = True if "csrf" in str(e).lower(): self.warning("Possible CSRF token mismatch, trying to obtain a new one...") self.refresh_csrf() return (False, 'too many errors') ``` New: ``` def send_comment(self, update, comment, karma, retries=3): while retries: try: res = self.client.comment(update["updateid"], comment, karma=karma) return (True, res) except BodhiClientException as e: self.warning("Bodhi Client error: %s" % str(e)) if "csrf" in str(e).lower(): self.warning("Possible CSRF token mismatch, trying to obtain a new one...") self.refresh_csrf() retries = retries - 1 return (False, 'too many errors') ``` If it wasn't rewritten, the bug wouldn't have been introduced. These are just a few examples (I have 27 detected problems to go through, and that's just what my AI found). I'll continue with the review tomorrow. But I'm not sure if fixing all of these is the easiest way forward...
adamwill commented 2026-02-18 18:29:44 +00:00
Owner
Copy link

We could of course dump the existing PR and generate it from scratch, but since I've already invested time into this review, it feels like wasted time to me

This feels like the sunk cost fallacy to me. If you think we did this wrong, and I think you made a good case for that, we should throw it away and do it again. It doesn't make sense to keep dumping more time into a review of a bad approach just because we already dumped some time into it. I think we should, indeed, start over with a new prompt.

> We could of course dump the existing PR and generate it from scratch, but since I've already invested time into this review, it feels like wasted time to me This feels like the [sunk cost fallacy](https://thedecisionlab.com/biases/the-sunk-cost-fallacy) to me. If you think we did this wrong, and I think you made a good case for that, we should throw it away and do it again. It doesn't make sense to keep dumping more time into a review of a bad approach just because we already dumped *some* time into it. I think we should, indeed, start over with a new prompt.
This tries to make the new code as much equivalent to the old code as possible,
for an easier review.
It was too long.
kparal commented 2026-02-19 13:22:21 +00:00
Owner
Copy link

Instead of starting from scratch, I used AI to redo the current code, keep the structure, but make it functionally and code-wise as close as possible (inside individual functions/methods) to the original source. It's a large diff, but I checked a large part of it and it really is the result that I imagined - code is just moved around, but mostly kept the same.

Actually, some nice changes are now reverted, e.g. getting rid of munch dependency. But I'd like to start just with the restructuring step right now, without any arbitrary code improvements. After this is reviewed and pushed, we can create further changes individually, like removing munch, using the logging library, etc etc. But those would be individual changes, and much easier to review, because the code wouldn't be all moved around. Sounds good?

@lruzicka Can you please review the current PR? If it looks OK to you (and if you find no runtime issue), we would squash this whole thing, push it, and then we can start making more improvements.

Instead of starting from scratch, I used AI to redo the current code, keep the structure, but make it functionally and code-wise as close as possible (inside individual functions/methods) to the original source. It's a large diff, but I checked a large part of it and it really is the result that I imagined - code is just moved around, but mostly kept the same. Actually, some nice changes are now reverted, e.g. getting rid of `munch` dependency. But I'd like to start just with the restructuring step right now, without any arbitrary code improvements. After this is reviewed and pushed, we can create further changes individually, like removing munch, using the logging library, etc etc. But those would be individual changes, and much easier to review, because the code wouldn't be all moved around. Sounds good? @lruzicka Can you please review the current PR? If it looks OK to you (and if you find no runtime issue), we would squash this whole thing, push it, and then we can start making more improvements.
kparal changed title from WIP: Refactor: Modularize monolithic codebase into clean architecture (v0.60) to Refactor: Modularize monolithic codebase into clean architecture (v0.60) 2026-02-19 13:22:47 +00:00
lruzicka commented 2026-02-24 11:20:20 +00:00
Author
Owner
Copy link

This looks good to me. Let's use this version and then continue on a step-by-step basis.

This looks good to me. Let's use this version and then continue on a step-by-step basis.
kparal manually merged commit 5c093ea48b into master 2026-02-26 10:29:44 +00:00
kparal deleted branch refactor/easy-karma 2026-02-26 10:30:04 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
kparal
Labels
Clear labels
  
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
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
lruzicka
kparal
4 participants
Notifications
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/fedora-easy-karma!72
No description provided.