Make risky-file-permissions rule does not ignore FQCN #1563
Conversation
|
Why not just adding both short and long names to the same lists? The new configuration seems overly complicated. I am curious if others share my opinion on that matter. 🤔 Other than this, thanks for raising it, we clearly need to fix it. |
I thought that much simple way at first but re-thought there might be a possibility of need to add more modules to take care in this rule. Also, similar fix may be required in other rules listing only short names of modules currently and smarter way to list modules might be needed, I guess. ;-) But, of course, I'll make change that if it's better to keep it simple other than taking care of future possibilities. I don't have strong opinion about it. |
| _FQCN_MODS_MAP: List[Tuple[str, List[str]]] = [ | ||
| ( | ||
| # fqcn prefix: | ||
| 'ansible.builtin', | ||
| # modules: | ||
| [ | ||
| # _modules: | ||
| 'assemble', | ||
| 'copy', | ||
| 'file', | ||
| 'replace', | ||
| 'template', | ||
| # _modules_with_create: | ||
| 'blockinfile', | ||
| 'lineinfile', | ||
| ], | ||
| ), | ||
| ( | ||
| 'community.general', | ||
| [ | ||
| # _modules: | ||
| 'archive', | ||
| # _modules_with_create: | ||
| 'htpasswd', | ||
| 'ini_file', | ||
| ], | ||
| ), | ||
| ] | ||
| _MOD_FQCN_PREFIX_MAP: Dict[str, str] = dict( | ||
| itertools.chain.from_iterable( | ||
| ((mod, prefix) for mod in modules) for prefix, modules in _FQCN_MODS_MAP | ||
| ) | ||
| ) | ||
|
|
||
| _MODULES = { | ||
| 'archive', | ||
| 'assemble', | ||
| 'copy', # supports preserve | ||
| 'file', | ||
| 'replace', # implicit preserve behavior but mode: preserve is invalid | ||
| 'template', # supports preserve | ||
| # 'unarchive', # disabled because .tar.gz files can have permissions inside | ||
| } | ||
| for mod in _MODULES.copy(): | ||
| _MODULES.add(f'{_MOD_FQCN_PREFIX_MAP[mod]}.{mod}') |
There was a problem hiding this comment.
This is overly complex. _MODULES should be a (frozen) set with all available options and that is it.
| _MODULES_WITH_CREATE = { | ||
| 'blockinfile': False, | ||
| 'htpasswd': True, | ||
| 'ini_file': True, | ||
| 'lineinfile': False, | ||
| } | ||
| for mod in _MODULES_WITH_CREATE.copy(): | ||
| _MODULES_WITH_CREATE[f'{_MOD_FQCN_PREFIX_MAP[mod]}.{mod}'] = _MODULES_WITH_CREATE[ | ||
| mod | ||
| ] |
There was a problem hiding this comment.
The same thing here. _MODULES_WITH_CREATE should be a plain dictionary with all options directly listed.
Thanks for your feedbacks! I pushed the simple version for the issue. |
|
@ssato oops, another merge would require more work for you :p |
Address the bug that the risky-file-permissions rule ignores the modules passed as FQCN like ansible.builtin.copy. Signed-Off-By: Satoru SATOH <[email protected]>
|
@ssato Now you have write access to the repository. This means you can create branches, perform reviews and even merge changes that received reviews. I encourage you to start using branches instead of your fork because it allows other cores to collaborate on them, making easier to fix small things and merge things faster. Again, thanks for helping with the linter! |
ssbarnea
changed the title
Thanks a lot! I'll do so later. |
Address the bug that the risky-file-permissions rule ignores the modules passed as FQCN like ansible.builtin.copy.
Signed-Off-By: Satoru SATOH [email protected]
Fixes: #1528