Skip to content
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

[WIP] Add foreman_ansible_roles module #753

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

branic
Copy link

@branic branic commented Mar 25, 2020

This PR adds a foreman_ansible_roles module.

This module will allow working with Foreman Ansible Roles.

Marking this PR as WIP, but I wanted to get it open to get feedback on the current (although limited) functionality.

@evgeni
Copy link
Member

evgeni commented Mar 25, 2020

Is state=fetch used to list the available roles on a proxy?

If so, I'd prefer if we could have one module for listing, and one for performing actions, as that's what current ansible best practice is.

@branic
Copy link
Author

branic commented Mar 25, 2020

Is state=fetch used to list the available roles on a proxy?

If so, I'd prefer if we could have one module for listing, and one for performing actions, as that's what current ansible best practice is.

@evgeni The fetch action lists the Ansible Roles that have been placed in /etc/ansible/roles and are available to be imported.

I don't mind making that action a separate module if that's the desired direction. I think the foreman_search_facts module will be able to be used for listing roles that have already been imported.

Would you like fetch to be its own module?

@branic branic force-pushed the foreman_ansible_roles_module branch from 0a38887 to 51f935a Compare March 26, 2020 05:37
@branic
Copy link
Author

branic commented Mar 26, 2020

I've renamed the module to foreman_anisble_roles_facts and implemented not only fetch but also list and show.

I'll limit this PR to just the fact gathering module and open another PR to implement the import, delete, and obsolete.

I'll start working on adding test cases.

@branic
Copy link
Author

branic commented Apr 7, 2020

@evgeni What is the recommended way to get files on the filesystem when running tests?

I need to place a test role in /etc/ansible/roles so that fetch will have data to report. I'll need this in the future for testing import functionality as well.

@branic
Copy link
Author

branic commented Apr 7, 2020

@evgeni One other question. During the test run I'm creating the Organization and Location, but I need to assign the location to the smart proxy. I'm not finding any functionality to do that. Is there functionality to do that?

@evgeni
Copy link
Member

evgeni commented Apr 20, 2020

@evgeni One other question. During the test run I'm creating the Organization and Location, but I need to assign the location to the smart proxy. I'm not finding any functionality to do that. Is there functionality to do that?

Nope, sorry. We have #149 open, and I think there even was a PR for it, but never got merged :(

@evgeni
Copy link
Member

evgeni commented Apr 20, 2020

@evgeni What is the recommended way to get files on the filesystem when running tests?

I need to place a test role in /etc/ansible/roles so that fetch will have data to report. I'll need this in the future for testing import functionality as well.

Sadly not. We have the same issue with Puppet modules too. In the past, I've been using the following snippet

- hosts: foreman
become: true
tasks:
- name: install puppet-prometheus
command: /opt/puppetlabs/bin/puppet module install puppet-prometheus
args:
creates: /etc/puppetlabs/code/environments/production/modules/prometheus/
- name: import puppet modules into Foreman
command: hammer proxy import-classes --puppet-environment production --name {{ ansible_fqdn }}

and adding a foreman host to the inventory that is my real Foreman installation, not nice, but at least documented. you can also do it by hand and write it in the header of the test playbook:

## To record cassettes using this playbook (record_hostgroup)
## some puppet classes need to be imported.
## See comments in #582 for details. To import the classes run:
##
## $ puppet module install puppet-prometheus
## $ hammer proxy import-classes --puppet-environment production --name $(hostname -f)
##

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

I'm afraid we managed to leave this PR around for too long and merged a huge refactoring since then. I'll provide suggestions how to update.

plugins/modules/foreman_ansible_roles_facts.py Outdated Show resolved Hide resolved
plugins/modules/foreman_ansible_roles_facts.py Outdated Show resolved Hide resolved
plugins/modules/foreman_ansible_roles_facts.py Outdated Show resolved Hide resolved
plugins/modules/foreman_ansible_roles_facts.py Outdated Show resolved Hide resolved
plugins/modules/foreman_ansible_roles_facts.py Outdated Show resolved Hide resolved
plugins/modules/foreman_ansible_roles_facts.py Outdated Show resolved Hide resolved
plugins/modules/foreman_ansible_roles_facts.py Outdated Show resolved Hide resolved
plugins/modules/foreman_ansible_roles_facts.py Outdated Show resolved Hide resolved
plugins/modules/foreman_ansible_roles_facts.py Outdated Show resolved Hide resolved
@branic branic force-pushed the foreman_ansible_roles_module branch from 9076ee5 to ac970aa Compare April 20, 2020 18:49
@mdellweg mdellweg changed the title [WIP] Add foreman_anislbe_roles module [WIP] Add foreman_ansible_roles module Apr 21, 2020
@evgeni
Copy link
Member

evgeni commented Apr 21, 2020

Thanks @branic! And once more, sorry for keeping you busy by catching up on changes we made to our helpers.

I've tried your module, and it works as advertised. However, I am a bit confused about the exposed workflow(s).

  1. The list/show actions effectively duplicate the functionality of the foreman_search_facts module. I do agree that "one module per resource" is easier for the users (and would even go so far to say we should implement that for all our resources), but at the same time it feels weird to have it in the same module as fetch which is about a completely different dataset.
  2. The fetch action is the really interesting feature here, as it allows us to find modules that are available for import.
  3. Having an action parameter in a "facts" module is rather confusing, as facts are informational not actionable and IMHO exposing the API action name to the user is too much.

So if I'd be king (I am not, lucky you), I'd name this module foreman_ansible_importable_roles_info¹ (or …available_roles? or switch roles and the term? not sure) and only make it implement the fetch action (thus dropping the parameter altogether). And then I'd add a second module foreman_ansible_installed_roles (or whatever) that will again has no action param, and will do either list or show, depending on full_details or not.

¹: there has been a huge movement in Ansible to move away from the "facts" term to "info" - see https://docs.ansible.com/ansible/latest/modules/list_of_all_modules.html how all _facts modules are marked as deprecated in favor of their _info siblings

@evgeni
Copy link
Member

evgeni commented Apr 21, 2020

Oh, and I didn't see any difference between list and show in my tests. What extra info should show expose?

@branic
Copy link
Author

branic commented Apr 21, 2020

Oh, and I didn't see any difference between list and show in my tests. What extra info should show expose?

I haven't seen any difference in list and show in my tests either. The API has both a list and a show endpoint which was why I implemented both.

hammer ansible roles list and hammer ansible roles show --id 1 return the same information, just in different formats.

 [root@satellite ~]# hammer ansible roles list
---|-----------|--------------------
ID | NAME      | IMPORTED AT        
---|-----------|--------------------
1  | test_role | 2020/03/26 17:19:31
---|-----------|--------------------


[root@satellite ~]# hammer ansible roles info --id 1
Id:          1
Name:        test_role
Imported at: 2020/03/26 17:19:31

@evgeni
Copy link
Member

evgeni commented Apr 21, 2020

Yeah. I think list and show are just default methods of the api and thus exist here. Show is cheaper if you only need to see one item. But in our specific case we only need the list IMHO.

@branic
Copy link
Author

branic commented Apr 21, 2020

For the workflow, I've been conflicted as well. The list/show functionality does overlap with the foreman_search_facts module. I implemented them in with the fetch as I was looking at the module as returning available roles that either could be imported or that already were.

I agree it does make sense to split the fetch and list/show into their own modules. As for having a dedicated list/show module is that a direction you would like the project to go or should users just rely on the foreman_search_facts to gather that information?

@evgeni
Copy link
Member

evgeni commented Apr 21, 2020

Long term I'd love to see us having separate "info" modules for the various resources, but it's far from a priority as long foreman_search_facts covers the same functionality with a slightly more complicated interface.

@branic
Copy link
Author

branic commented Apr 21, 2020

Great. I'll work on splitting the module into two modules.

Thanks for all the great feedback.

- The Smart Proxy name to use for fetching Ansible Roles
required: false
type: str
aliases:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
aliases:
aliases:

(the linter is unhappy)

proxy:
description:
- The Smart Proxy name to use for fetching Ansible Roles
required: false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
required: false
required: true


DOCUMENTATION = '''
---
module: foreman_ansible_roles_facts
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
module: foreman_ansible_roles_facts
module: foreman_ansible_roles_importable_info

Comment on lines +88 to +89
organization=dict(type='entity'),
location=dict(type='entity'),
Copy link
Member

Choose a reason for hiding this comment

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

are those two actually used for anything? I know you can pass them in the request, but I don't see any use for doing so. the data on disk (which this action lists) is not org/loc aware.

I think the API just allows to pass those, as it allows this on every request.

Copy link
Member

Choose a reason for hiding this comment

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

@ares is that right? 'fetch' should not be scoped?


module = ForemanAnsibleRolesImportedInfoModule(
foreman_spec=dict(
search=dict(default=""),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
search=dict(default=""),
search=dict(),

no need for a default

module = ForemanAnsibleRolesImportedInfoModule(
foreman_spec=dict(
search=dict(default=""),
full_details=dict(type='bool', aliases=['info'], default=False),
Copy link
Member

Choose a reason for hiding this comment

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

given the API does not return more info, shall we drop that param?


DOCUMENTATION = '''
---
module: foreman_ansible_roles_facts
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
module: foreman_ansible_roles_facts
module: foreman_ansible_roles_imported_info

@evgeni evgeni changed the base branch from master to develop July 24, 2020 08:27
@evgeni evgeni added this to the puppet to ansible migration milestone May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants