Add Foreman inventory #19510

dLobatog · 2016-12-19T11:49:48Z

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

external inventories

ANSIBLE VERSION

ansible 2.2.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides

SUMMARY

Adds an inventory to the list of external inventories in contrib. This inventory allows Ansible users to query information from Foreman and Satellite 6 (https://github.com/theforeman/foreman_ansible_inventory).

Should I add the README here too?

chrismeyersfsu · 2016-12-19T14:55:49Z

@dLobatog please sprinkle the docs https://github.com/theforeman/foreman_ansible_inventory into the .ini file.

abadger · 2016-12-19T15:31:08Z

contrib/inventory/foreman.py

+        self.facts = dict()   # Facts of each host
+        self.hostgroups = dict()  # host groups
+
+    def run(self):


Typically we organize code so that callers are closer to the bottom and callees are near the top (__init__ being the exception... it's always at the top). This organization puts methods like run() near the bottom of the class and helpers above it.

abadger · 2016-12-19T15:32:16Z

contrib/inventory/foreman.py

+import os
+import re
+import requests
+from requests.auth import HTTPBasicAuth


Note, we try to organize imports into three blocks (all optional). stdlib imports at the top, then third party library imports (requests in this case), and finally any imports from ansible itself.

abadger · 2016-12-19T15:34:52Z

contrib/inventory/foreman.py

+        cache.write(json_data)
+        cache.close()
+
+    @staticmethod


Helper functions like this (Ones that don't use self. Ones that could be marked @staticmethod if they were part of the class). should typically be placed at the toplevel. So far I see to_safe, json_format_dict, and push.

abadger · 2016-12-19T15:39:37Z

contrib/inventory/foreman.py

+try:
+    import json
+except ImportError:
+    import simplejson as json


The json library entered the python stdlib with python-2.6. Code that runs on the controller (which includes dynamic inventory scripts) need python-2.6 or greater. So there's no need to fallback to simplejson here. the json stdlib library should always be available.

abadger · 2016-12-19T15:39:56Z

contrib/inventory/foreman.py

+try:
+    import ConfigParser
+except ImportError:
+    import configparser as ConfigParser


Comment to explain python2 vs python3 would be good here.

abadger · 2016-12-19T15:48:09Z

contrib/inventory/foreman.py

+
+
+class ForemanInventory(object):
+    config_paths = [


This doesn't really matter since only a single instance of ForemanInventory is created in this script but config_paths should really be an instance attribute (created and given an initial value in __init__) rather than a class attribute. Class attributes exist a single time per class. All instances of the class would share the same instance of that attribute. Instance attributes exist, one per instance and thus can change independently. Modifying config_paths later in the code is a tip-off that this should be an instance-attribute rather than a class-attribute.

Also, you can probably move the setting from an environment variable to be with this code when you do that. It would seem to make sense to keep that all together.

bcoca · 2016-12-19T16:08:25Z

contrib/inventory/foreman.py

+import copy
+import os
+import re
+import requests


it would be better to avoid the requests lib, but if you cannot you should at least specify a min version.

abadger · 2016-12-19T16:58:14Z

Made some easy to fix style comments. The main logic looks like it will do what it's supposed to. It looks like there could be some larger changes that could be made (for instance, I think that --host only affects output here... It does not affect what has to be fetched from foreman). As long as you're continuing to maintain it, I don't think that's a blocker for getting this merged... more of a note about what could be fixed in the future.

abadger · 2016-12-19T16:58:37Z

needs_revision

dLobatog · 2016-12-21T16:55:54Z

@abadger Thanks for taking a look - not a Python expert myself, but I think I've addressed most of the comments. The one I wasn't so sure about is @bcoca 's ask to pin requests to a version. I think 1.1 (what CentOS 6 has) is probably safe, but I'm not sure if you wanted something different.

Hope this works - I'll see if I can remove requests from here at some point even, it was just.. convenient at the time. Thanks @chrismeyersfsu @abadger @bcoca for taking a look

dLobatog · 2016-12-21T17:12:04Z

BTW CI is picking on a problem on ./lib/ansible/modules/network/ldap_entry.py , not foreman.py

abadger · 2017-01-05T01:48:22Z

contrib/inventory/foreman.py

+        regex = "[^A-Za-z0-9\_]"
+        return re.sub(regex, "_", word.replace(" ", ""))
+
+    def json_format_dict(self, data, pretty=False):


This should be a @staticmethod and self removed.

Actually, better would be to move this to the toplevel as a regular function

abadger · 2017-01-05T01:48:36Z

contrib/inventory/foreman.py

+        else:
+            return json.dumps(data)
+
+    def push(self, d, k, v):


This should be an @staticmethod nad self removed.

Same here, make this a regular function

Actually... Even better: use a defaultdict instead of this. This is how defaultdicts work:

from collections import defaultdict foo = defaultdict(list) foo[k].append(v)

abadger · 2017-01-05T01:49:20Z

contrib/inventory/foreman.py

+        else:
+            d[k] = [v]
+
+    def run(self):


This should probably be the last method in the class.

abadger · 2017-01-05T01:51:14Z

contrib/inventory/foreman.py

+            self.config_paths.append(os.path.expanduser(os.path.expandvars(env_value)))
+
+    @staticmethod
+    def to_safe(word):


Make this a regular function

abadger · 2017-01-05T02:00:16Z

contrib/inventory/foreman.py

+        self._print_data()
+        return True
+
+    def _read_settings(self):


This method doesn't seem to do much. I would merge it into the run() method.

read_settings and parse_cli_args don't seem related. They don't operate on the same instance attributes, for instance.

It's a bit funny to have a private method (_read_settings()) calling public methods (read_settings() and parse_cli_args()) that aren't usable by anything else.

If this is merged into run(), it will only add one more line there and will remove 5 lines overall.

abadger · 2017-01-05T02:10:52Z

contrib/inventory/foreman.py

+        except (ConfigParser.NoOptionError, ConfigParser.NoSectionError):
+            group_patterns = "[]"
+
+        self.group_patterns = eval(group_patterns)


I didn't notice this before but you have eval here... What is that for? It's rather dangerous as it means if someone can get code into the config file for the foreman dynamic inventory, they can then execute whatever code they want as the user running ansible. We should get rid of this if at all possible.

abadger · 2017-01-05T02:12:25Z

contrib/inventory/foreman.py

+        self.parse_cli_args()
+        return True
+
+    def _get_inventory(self):


No need for this to be a private method. rename to get_inventory().

abadger · 2017-01-05T02:24:17Z

contrib/inventory/foreman.py

+    def _get_hosts(self):
+        return self._get_json("%s/api/v2/hosts" % self.foreman_url)
+
+    def _get_hostgroup_by_id(self, hid):


This doesn't seem to be called from anywhere. Should it be deleted?

Yes, it was used to resolve Foreman host parameters on theforeman/foreman_ansible_inventory@9972f3f#diff-2b3c7ba3ec283a3b380cff70f61e0290R144 , not needed anymore

abadger · 2017-01-05T02:30:42Z

contrib/inventory/foreman.py

+        if self.args.refresh_cache:
+            self.update_cache()
+        elif not self.is_cache_valid():
+            self.update_cache()


Probably better to write this as

if self.args.refresh_cache or not self.is_cache_valid(): self.update_cache()

abadger · 2017-01-05T02:35:44Z

This is probably what the order of methods should be inside of the class:

init()
read_settings()
parse_cli_args()
_get_json()
_get_hosts()
_get_all_params_by_id()
_resolve_params()
_get_facts_by_id()
_get_facts()
write_to_cache()
_write_cache()
update_cache()
is_cache_valid()
load_inventory_from_cache()
load_params_from_cache()
load_facts_from_cache()
load_cache_from_cache()
_get_inventory()
get_host_info()
_print_data()
run()

This puts the entrypoint at the bottom and then everything that helps it to run is above it.

abadger · 2017-01-05T02:37:46Z

Okay, I think I've gone over this sufficiently that this is everything that needs to be changed.

dLobatog · 2017-01-23T21:24:33Z

@abadger Thanks so much for the detailed review! I think I've gone through each of the things you've commented on , with the exception of #19510 (comment) - I didn't know what you referred to, it looks like a regular function already?

The latest push contains the updates to your comments, including order of methods, and the improvements @wenottingham made here theforeman/foreman_ansible_inventory@7a55c62

abadger · 2017-01-31T00:57:06Z

@dLobatog Right now it's a method. My comment is to make it a function instead.

This commit adds the foreman inventory based on https://github.com/theforeman/foreman_ansible_inventory and its configuration file.

dLobatog · 2017-02-09T15:50:45Z

@abadger Updated - the json_format_dict function is now out of the ForemanInventory class. I wasn't sure where to put it so I put it on top. Thanks for the review!

abadger · 2017-02-21T17:21:42Z

Merged to devel for inclusion in 2.3.0. Thaks @dLobatog !

dLobatog · 2017-02-21T21:43:12Z

Thanks @abadger for the review!

dLobatog force-pushed the foreman-inventory branch from e5ccc10 to 734b77f Compare December 19, 2016 12:40

ansibot added affects_2.3 This issue/PR affects Ansible v2.3 feature_pull_request labels Dec 19, 2016

abadger reviewed Dec 19, 2016

View reviewed changes

dLobatog force-pushed the foreman-inventory branch from 734b77f to ac2fc49 Compare December 19, 2016 15:32

abadger reviewed Dec 19, 2016

View reviewed changes

ansibot added needs_triage Needs a first human triage before being processed. and removed triage labels Dec 19, 2016

abadger reviewed Dec 19, 2016

View reviewed changes

bcoca reviewed Dec 19, 2016

View reviewed changes

bcoca removed the needs_triage Needs a first human triage before being processed. label Dec 19, 2016

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 19, 2016

dLobatog force-pushed the foreman-inventory branch from ac2fc49 to f1612e7 Compare December 21, 2016 16:42

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 21, 2016

dLobatog force-pushed the foreman-inventory branch from f1612e7 to eef6b66 Compare December 21, 2016 16:54

dLobatog force-pushed the foreman-inventory branch from eef6b66 to 12216b2 Compare December 21, 2016 17:01

abadger reviewed Jan 5, 2017

View reviewed changes

contrib/inventory/foreman.py Outdated

else:

d[k] = [v]

def run(self):

Copy link

Contributor

abadger Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be the last method in the class.

abadger reviewed Jan 5, 2017

View reviewed changes

alikins added the c:inventory/contrib_script label Jan 5, 2017

wenottingham mentioned this pull request Jan 12, 2017

Fix some things to make inventory script perform better. theforeman/foreman_ansible_inventory#41

Merged

dLobatog force-pushed the foreman-inventory branch from 12216b2 to f2bf34c Compare January 23, 2017 21:22

Add Foreman inventory

ac79854

This commit adds the foreman inventory based on https://github.com/theforeman/foreman_ansible_inventory and its configuration file.

dLobatog force-pushed the foreman-inventory branch from f2bf34c to ac79854 Compare February 9, 2017 15:48

abadger merged commit 01436cf into ansible:devel Feb 21, 2017

dLobatog mentioned this pull request Mar 28, 2017

Point new users to ansible/ansible theforeman/foreman_ansible_inventory#45

Closed

2 tasks

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018

dagwieers added the foreman Foreman community label Feb 8, 2019

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Foreman inventory #19510

Add Foreman inventory #19510

dLobatog commented Dec 19, 2016

chrismeyersfsu commented Dec 19, 2016

abadger Dec 19, 2016

abadger Dec 19, 2016

abadger Dec 19, 2016

abadger Dec 19, 2016

abadger Dec 19, 2016

abadger Dec 19, 2016

bcoca Dec 19, 2016

abadger commented Dec 19, 2016

abadger commented Dec 19, 2016

dLobatog commented Dec 21, 2016

dLobatog commented Dec 21, 2016

abadger Jan 5, 2017

abadger Jan 5, 2017

abadger Jan 5, 2017

abadger Jan 5, 2017

abadger Jan 5, 2017

abadger Jan 5, 2017

abadger Jan 5, 2017

abadger Jan 5, 2017

abadger Jan 5, 2017

abadger Jan 5, 2017

abadger Jan 5, 2017

dLobatog Jan 23, 2017

abadger Jan 5, 2017

abadger commented Jan 5, 2017

abadger commented Jan 5, 2017

dLobatog commented Jan 23, 2017

abadger commented Jan 31, 2017

dLobatog commented Feb 9, 2017

abadger commented Feb 21, 2017

dLobatog commented Feb 21, 2017

Add Foreman inventory #19510

Add Foreman inventory #19510

Conversation

dLobatog commented Dec 19, 2016

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

SUMMARY

chrismeyersfsu commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadger commented Dec 19, 2016

abadger commented Dec 19, 2016

dLobatog commented Dec 21, 2016

dLobatog commented Dec 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadger commented Jan 5, 2017

abadger commented Jan 5, 2017

dLobatog commented Jan 23, 2017

abadger commented Jan 31, 2017

dLobatog commented Feb 9, 2017

abadger commented Feb 21, 2017

dLobatog commented Feb 21, 2017