-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added template verification playbook #1
Conversation
No problem. I need more time to review this, but on the surface most of it makes sense. Can you clarify anything on your .yml files and roles? Are these meant more for testing or when users will actually use the ntc_show_command module? Again, I only took a real quick look and will try and dive deeper within the next 1-2 days. Thanks! |
Thanks Jason. |
Changes to argument handling
Yes, understand roles, just wanted more context :) Slowly going through this...can you remove cisco_show_ip_int_brief.raw from ntc_templates? It should only be in the tests dir. Would you mind also creating a separate directory for each template in tests, i.e. a dir called cisco_show_ip_int_brief, cisco_show_vlan, etc.? That should clean things up. |
Ok, so i've cleaned up all the redundant files. so ntp_temlpates will only have templates and tests will have a vendor directory and command directory within it, each with two files - raw and parsed. Also I got rid of test-specific definitions in favour of vendor-specific definitions. All files now assume a strict naming convention of |
--- | ||
- name: verify that parsed result equals expected sample | ||
compare_dict: | ||
result: "{{ vlans.response }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please verify this is accurate - should this be vlans.response
or something more abstract?
FYI- Just commented inline on the repo in a few locations. |
yep, cool, agreed. It did feel like a bit of over-engineering when i was doing it. 4 roles for testing and none for the actual module. so cool, i'll collapse all testing in a single role. that "vlan.response" is actually returned by your module and i was planning to fix it in a separate branch but can fix it here, no issues. I'll also update contributing.md to clarify instructions on how to add test cases for templates. should get it pushed later today. |
Would you mind adding .pyc files to the .gitignore and then removing yours from this request? Is After this, I think we'll be good...just lots of files to review :) BTW, based on your testing platform, can you update the template names to cisco_nxos_command or cisco_ios, etc. See other PR coming in...outputs are different, and we are going to try and go with template name to match the netmiko vendor type and then _command. Just update your template name for now though and other changes will soon follow nice work!!! |
All done. I've updated tests as well as template files (including index). I haven't touched that main site.yml. |
Think it's good...I'll try and test more over the coming days and may update as necessary! Thank you very much!! |
added template verification playbook
Cool. Thank you too, mate. In the future, if this project takes off, you would be able to completely automate the addition of new templates with a CI server and a test playbook. |
Reword Documentation in attempt to make parameters more clear
Hey Jason,
i hope this doesn't feel like intrusion but i've allowed myself the liberty of adding a template testing functionality to your project. There's a few changes I've made
./tests
directory to store .raw files, .parse files and .yml files for each templateand finally, i've added a template (along with the tests) for 'show ip int brief' command.
I hope I didn't come off rude or anything. This is my first pull request and I don't have much experience in collaborative development so apologies if i did anything wrong.