Hi,
along my discussions on adding possibility to handle multiple keys using the authorized_key module, I realised the functionality was hidden in the code, but since it was supported without being documented I felt the code had organically grown a little strange. I took liberty to refactor the code: https://github.com/ansible/ansible-modules-core/pull/409
The pull request impacts around 70 lines so maybe not that easy to review before merging on such a critical module. What could I do to make the merging easier?
Fabrice
For starters, I would include some comments int he pull request about what the change changes, and what new features it provides, etc, so we don’t have to try to infer this from the source code.
Thanks!
Hi Michael,
to be honest this pull request does not add new features. It is just a refactoring to make the module much simpler before adding any features.
I see two things I could do:
- separate the pull request in multiple commits to make each one very simple
- comment on the pull request what each change does
One thing that bothers me is that my IDE automatically removed trailing spaces which makes the pull request less readable, that could be a first step, maybe?
Let me know what makes your life the easiest.
Fabrice
Ok, we are really not interested in refactoring pull requests.
I understand this sounds harsh, but if it doesn’t add a feature, we need to prioritize other things that do, or that fix bugs, and refactoring pull requests can only introduce bugs.
Refactoring as a result of adding a small feature is definitely fair game, but with something like authorized_key, we are going to err on the virtue of being fairly conservative. I do understand all submitted code is not super pretty – but that’s kind of where we are at.
If there are specific problems, of course, we want to address them.
Hi Michael,
I totally understand your point, this is why I suggested making it the easiest possible to review my pull request on such a sensitive feature. Two points to defend this pull request :
- I did see it as a first step to add small features afterwards
- And on such a security-sensitive module, rewriting the code from messy (some variables are not even used, there are too many ifs compared to the only three possible choices, etc.) to the very simple version I ended up with is a security feature imho
Of course I checked that the tests pass before submitting.
Once again I understand your point, I am the first to put bugfixing and new features before refactoring. I think however that in this case I highly improved the auditability of a sensitive module, in the perspective to add features afterwards.
If I promise to directly add features in my future contributions and make it worth your time, does that make you reconsider your policy on this first contribution? 
Thanks,
Fabrice
I think changing it when it doesn’t have any known vulnerabilities is not a security feature, though I also don’t see explicit examples of things that you may find problematic.
If you do, however, email us with security@ansible.com.
Unfortunately we are going to prioritize some other things at this time, I think it’s fine to leave it in queue, but I can’t give you any immediate time table on when it may be included or reviewed.
Generally we have declined refactoring-specific PRs in the past because they usually invalidate other PRs, but do spend a lot of time on refactoring (such as the v2/ subtree project) from time to time.
Though we strongly feel we must deeply understand sources, and shifting it all around is not a 0-cost exercise to us, in particular, even the review costs singificant time that can be applied elsewhere.
I guess my advice is add a feature, and keep refactorings to things around areas that make adding that feature easier.
DRY cases can also be good cases for a PR, for instance, when we noticed a lot of reused snippets in various cloud modules, it makes sense to move that to one place.