search

pearofducks / ansible-vim

A vim plugin for syntax highlighting Ansible's common filetypes
MIT License
800 stars 98 forks source link

use fully qualified collection name (FQCN) for generated snippets #118

Closed pescobar closed 3 years ago

pescobar commented 3 years ago

in the latest ansible versions it's recommended to use the fully qualified collection name (FQCN) for modules e.g. ansible.builtin.yum instead of just yum . This is a quick&dirty PoC to include the FQCN in the generated snippets.

I guess to do it in a cleaner way it would require a bigger refactor of the script. By now I tried to don't modify your existing functions and feed them with lists.

This PoC has a weird problem which I couldn't fix yet. I don't know why when I do ansible-galaxy collection install openstack.cloud and I execute the script generate.py --user the generated file ansible.snippets includes the openstack modules twice. I don't get why. Probably I am doing some stupid mistake.

It would be nice if the collection name could be queried directly using ansible.utils or ansible.plugins but in a quick search I couldn't find how to do it and I ended up parsing the path in the new function get_collection_name() . Do you know if there is any better alternative to get the collection name?

If you are open to merge this I can try to improve it. I wanted to get your opinion about how it should be done before investing more time and do a bigger refactor that maybe you know how to do better.

pescobar commented 3 years ago

I have updated get_docstrings() and renamed it to get_module_docstring() so it can query the docstring for a single ansible module. Then I have added some extra logic in main() to query the builtin modules and the user modules independently.

I have also realized that I was getting duplicated entries for collection openstack.cloud because it includes some symlinks for legacy module names starting by os_ so I updated function get_files() to ignore symlinks in this commit

I guess now it's working. It could be optimized to execute get_files() only once to save disk accesses but as this script is not executed too often maybe it's not worth doing it

pearofducks commented 3 years ago

Thanks for the work here! Can you ping me when it's ready for review? You've had commits the past few days, so I'm not sure when it's considered ready vs. WIP.

pearofducks commented 3 years ago

BTW - if this has breaking changes, at worst we can keep both and this can become the defacto solution and the other script can be renamed but still available.

pescobar commented 3 years ago

@pearofducks please review it when you have some time. I did some tests and the current version is working fine for me. I tested it mainly with ansible 2.10.6

I added this commit to only add the FQCN when the ansible version is 2.10 or higher so I guess it should be backward compatible. I did some tests with ansible 2.9.13 and 2.10.6 and it seems to work as expected. BTW, I have noticed that when executing the script with ansible 2.9.13 the generated ansible.snippets is around 50K lines. Do you know if this can have an impact in vim's performance?

Let me know if you want any change or improvement.

pescobar commented 3 years ago

I did another change splitting get_files() in two different fuctions get_files_builtin() and get_files_user() . I promise this is the last change :) Please let me know what you think.

p.s. I am usually online in freenode with the same nick in case you want to discuss it "live"

pearofducks commented 3 years ago

Really nice work here @pescobar - I appreciate you putting this together and the extra measures you've put in to ensure backward compatibility!