Update Ansible Command

[PhillSimonds]

This PR Updates the schema-enforcer ansible command per #7, #34, #44 and and #47

Overview

• Documentation has been updated in the docs/ansible_command.md file per Validate/update doc strings and README #7
• The following additional configuration settings have been implemented. They're documented in the afformentioned doc.
  • schema_enforcer_schema_ids -- This was updated from schema_enforcer_schemas to schema_enforcer_schema_ids for clarity
  • schema_enforcer_strict
  • schema_enforcer_automap_default -- This was updated from schema_enforcer_automap for clarity.
• All commands have been validated with ansible 2.10.3 per Validate ansible command with 2.10 #34

The following were resolved and do not have an issue -- LMK if it would be best to create one. Since this was a total overhaul of the ansible command, I didn't know if it would be overkill to create specific issues for it right now.

• A --show-checks argument has been added to the schema-enforcer ansible command to show the schemas to which each host will be checked for adherence.
• A bug in the schema-enforcer ansible command which made it not actually evaluate schemas correctly has been remediated.

Worth Noting

I added a few host-specific methods to the AnsibleInventory class as there were already a few host-specific methods which already existed on it. The AnsibleInventory class is very similar to the InstanceFileManager class, except we have an InstanceFile class as well. Many instances of the InstanceFile class become attributes of the InstanceFileManager class. Methods specific to an individual instance file are created on the InstanceFile class, where methods that apply to many instance files are implemented on the InstanceFileManager class.

The AnsibleInventory class breaks away from this logic by implementing host-specific methods as well as those that are inventory specific on the same AnsibleInventory class itself, then expecting a Host object as input to the methods defined. It may make sense to create a specific AnsibleHost class by subclassing ansible.inventory.host.Host. This would allow us to implement host-specific functions on a single host object (e.g. AnsibleHost) and inventory specific functions on the AnsibleInventory class, following the same architectural paradigm in use by the InstanceFileManager and InstanceFile classes. The tradeoff to this is that we'd have to override some built in functions which Ansible is currently providing, namely the population of hosts. I'm not sure what to do here, but thought it was worth bringing up that there is a divergence between the two architectures in the code base and that difference of approach creates an inconsistency which could lead to confusion on where to modify current methods, and where to put future methods..

TODO

• Add unit tests. I wanted to 1) solicit y'all for guidance on the architectural question in the Worth Noting section and 2) Validate that the way I implemented the code makes sense before I write the unit tests. I know this is contrary to TDD, but I've lately spent a lot of time un-necessarily re-writing unit tests to fit, then refit, then refit again a new architecture because the architecture wasn't well defined/ratified before I wrote them, so wanted to avoid that. -- updated, will add unit tests in another PR.
• Update main README.md with some info on the ansible command
• Update ansible command doc strings + comments in cli.py

[itdependsnetworks]

Great stuff!!! A bunch of little nitpicks but great all around.

[PhillSimonds]

Thanks for the review! I've updated some of the basics and will finish updating a couple of the other things which need just a little more testing tomorrow AM. Do you have any thoughts on the Worth Noting section in the main body of the PR overview?

[PhillSimonds]

@itdependsnetworks I removed the examples of passing in dynamic inventory and created Issue #55 to track a resolution for using dynamic inventory. The examples put together required that the dynamic inventory script directory be specified as an environment variable -- I'm thinking we'll want to ratify a strategy for how to declare this in a way that's a little cleaner than requiring the user to pass in an environment variable before execution. I'm thinking to add those examples back in inside of their own section in the README.md after we ratify a strategy and add the feature for dynamic inventory parsing.

This PR should be ready to merge, spare unit tests. @dgarros, can you give me a yay or nay on how the code is implemented before I write those :)?

[dgarros]

Looks great

[PhillSimonds]

Thanks for the review Damien! @itdependsnetworks -- are you good with us merging this?

[itdependsnetworks]

Yea, looks good to me.

[jmcgill298]

I am confused if this is testing the assimilation of data per ansible inventory, or just a particular file? If it is a file, how does it know what file to look for? How does this work with dynamic inventory?

How does defining the same data within multiple groups or group_vars/host_vars affect this?

[PhillSimonds]

I've updated some of the verbiage to make this a little more clear.

• No matter where the inventory is rendered from, you end up with a data structure of the defined variables per host. This per-host data is what is validated for schema adherence on a per host basis.
• The tool doesn't yet officially support dynamic inventory. We've tested it and had it working, but we had to set environment variables to do so. I created an issue (Add options for parsing Ansible dynamic inventory #55) to track adding this feature in as I'd like have how a dynamic inventory is configured for use and how it's constructed clearly designed, documented, and articulated with it's own section and examples.
• The flattening of vars per host is done per ansible's rules for doing so

[jmcgill298]

Thanks for clarifying that for me. I went back and re-read it after the updates, and I would say that there are still 3 things that cause me not to read the docs that way, but to read it as validating the variables associated with a file

• The near identical schema and file names (makes sense that they have similar names, and not a problem if the next point is agreed upon)
• There is a single variable file hosting all of the data
• There aren't any ansible variables in the vars/schema (ansible_user, ansilbe_password)

I think adding an additional vars file (host or group) and adding some common variables used by the ansible framework would help push the reader into the interpretation you have defined above.

[PhillSimonds]

Thanks for the input. Agreed. I'm going to go ahead and close this PR and submit another one with the example you've referenced in the interest of getting this one buttoned up :).