feat(refactor): Ansible Role for Improved Code Quality and Maintainability#275
feat(refactor): Ansible Role for Improved Code Quality and Maintainability#275Tealk wants to merge 5 commits intopaperless-ngx:mainfrom
Conversation
This should fix the problem described in issue paperless-ngx#263.
Replace deprecated punkt with punkt_tab in NLTK downloader task
fix python check
|
Hi @Tealk thanks for your huge effort. Would you mind to send all logical packets as separate packets so that they all can be reviewed and documented on their own? This is a huge PR which needs some dissection. From a quick overview: Personally I am against the logical ordering of variables. That caused a lot of trouble in the past to keep consistency and clean separated blocks of vars. I'd like to stick with alphabetical order (which I use for the variables in multiple files) that will always be valid ;) Also the logic comes from the ansible main repo, in this repo it should only be of interest if a var is already supported. |
|
Hi, I dont understand what you mean with:
What do you mean by splitting it up? |
|
Hi @Tealk , sorry for the confusion. What I meant was, that the meaning of each variable and to which broader scope (database etc.) it belongs to is given by the help page of the paperless-ngx main repo. This paired with the experience I gathered by trying to keep such logical blocks/scopes for variables consistent over time in this ansible files makes me not a fan of reintroducing it :-) I would like to keep it alphabetically sorted. Given this example and my wish to keep traceability of what PR introduced what particular change I would like to ask you to provide different PRs for each attempt of making the repo better. The NLTK PR is clearly a fix. Others are improvements to dedicated portions of the code. Some might be discussworthy, others will be merged easily (like the python fix you provided). You think this is possible? |
|
I currently wouldn't know how to break this down. I first ran a strict linting process over it and fixed all errors and warnings, and adjusted dependencies. I recreated the config because I couldn't figure it out with the little info in the readme and I had to search through the code to see what each variable actually does. You're welcome to take out what you like and leave what you don't. I'm happy to test again, but currently the version in this repo isn't stable enough for a production environment for me. |
|
Yes, I completely feel you. But the PR is huge and combines a lot of different improvements. If you say it is not stable enough for production, can you elaborate a bit more on this? I merged the two other PRs you submitted. That would leave the NLTK thing a third that is needed. For that one I will see why the test suites did not report this issue and will adapt. Besides that anything else that is needed for production usage? Regarding the variables: Would it have helped you if I referenced the paperless manual page for quick lookups? Because there it is clearly stated what each var does. Specific ones to the role (which are few) are documented in the Readme. Or do I miss a point here? |
|
Unfortunately, I can't tell you exactly which tasks caused problems anymore, as it's been too long. There were a few scenarios that didn't run cleanly and where manual intervention was needed to get the playbook running again. I only adjusted the variables because I had cross-checked them while making adjustments and it was too confusing for me; especially since only the first 50% or so were sorted alphabetically. I can't understand why you had problems with the sorting there, I also have some larger config files that I sort exactly the same way and haven't had that problem before. |
I wouldn't call it problems in the first place. It is recurring effort. Because if I introduce a ordering system then it must be consistent to the categories here: https://docs.paperless-ngx.com/configuration/ And these switched at least two times over the years on a bigger scale. That's why I gave up to follow. And I think most of the people just do look up the vars there. |
An even bigger reason to keep track of each single one ;) For the NLTK thing: Seems like it really boils down to switching the package from punkt to punkt_tab? Or is uninstall of the old needed? Anything else? |
Since I've deleted the directories quite often, I can't answer that at all right now; but I believe he ignored the old package. |
Overview
This pull request comprehensively refactors the Paperless-ngx Ansible role to improve code quality, readability, and maintainability. The changes focus on better organization, consistent formatting, and modernized Ansible syntax patterns.
in this PR #271 #272 and #273 would already be included
Key Changes
1. Default Variables Reorganization (
defaults/main.yml)2. Code Quality Improvements
...)3. Specific Enhancements
4. Ansible Best Practices