Add content export library to FAM

[jeremylenz]

Adds pulpcore content exports to Foreman Ansible Modules.

Covers exports of

• library
• ~~content view version~~
• ~~repository~~

Examples cover creation of metadata.json which is required to import the exported content.

Does not cover:

• imports
• syncable exports

Todo:

• [x] export history info module
• [x] fix tests
• [x] show metadata.json creation in examples

To test:

sudo make dist-test

ansible-playbook tests/test_playbooks/content_export_info.yml

[evgeni]

So, I played a bit a round with this, and this patch makes it a bit tidier:

diff --git plugins/modules/content_export.py plugins/modules/content_export.py
index a5d1b615..3811da38 100644
--- plugins/modules/content_export.py
+++ plugins/modules/content_export.py
@@ -32,7 +32,7 @@ author:
 '''
 
 
-from ansible_collections.theforeman.foreman.plugins.module_utils.foreman_helper import KatelloAnsibleModule
+from ansible_collections.theforeman.foreman.plugins.module_utils.foreman_helper import KatelloAnsibleModule, _flatten_entity
 
 
 class KatelloContentExportModule(KatelloAnsibleModule):
@@ -49,15 +49,8 @@ def main():
     )
 
     with module.api_connection():
-        scope = module.scope_for('organization')
-        # payload = {
-        #     'destination_server': module.foreman_params['destination_server'],
-        #     'chunk_size_gb': module.foreman_params['chunk_size_gb'],
-        #     'fail_on_missing_content': module.foreman_params['fail_on_missing_content'],
-        # }
-        payload = {key: module.params[key] for key in module.params.keys() if key in module.foreman_params}
-        payload.update(scope)
-        print(payload)
+        module.auto_lookup_entities()
+        payload = _flatten_entity(module.foreman_params, module.foreman_spec)
         task = module.resource_action('content_exports', 'library', payload)
 
         module.exit_json(task=task)

[jeremylenz]

@evgeni here's the link to the docs: https://docs.theforeman.org/nightly/Content_Management_Guide/index-katello.html#Synchronizing_Content_Between_Servers_content-management

[jeremylenz]

As far as how to divide it up, what I was thinking is

3 modules in this PR, each covering both full and incremental:

• content_export_library
• content_export_repository
• content_export_version

and a corresponding follow-up card for Import. @parthaa does that sound good?

[parthaa]

and a corresponding follow-up card for Import. @parthaa does that sound good? Sounds good to me

[wbclark]

As far as how to divide it up, what I was thinking is

3 modules in this PR, each covering both full and incremental:

* content_export_library

* content_export_repository

* content_export_version

and a corresponding follow-up card for Import. @parthaa does that sound good?

I think it should be doable to have all this within a single module. The parameters could include a Boolean for Library. If that is not set to True then it would look either for a Repository name, or a CV name and version ID, or a CV name and lifecycle environment.

For an example of what conditionally required parameters look like, see: https://github.com/theforeman/foreman-ansible-modules/blob/d77a0f1162142022e69110ad4cdd692c8d2c15c5/plugins/modules/bookmark.py#L141-L143

You can also make parameters mutually exclusive, as in: https://github.com/theforeman/foreman-ansible-modules/blob/a1330d3a6d1867eb7b7112d0d030ad0439babd6b/plugins/module_utils/foreman_helper.py#L1345

Per https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html there is also required_one_of, required_together, etc. So there are powerful tools to specify what combinations of data the module will accept, and then you can write the module assuming that a "correct" input was received and specifying the endpoints appropriately.

[evgeni]

I think it should be doable to have all this within a single module.

We could. But from talking to Jeremy and reading the docs, the three workflows (library, version and repo exports) seem to be solving completely different user scenarios and the only thing they have in common is the fact that the end result is a "content export". Given the workflows are different (and so are the hammer commands: hammer content-export complete version - not hammer content-export complete --type version), it feels more natural (to me) to have different modules, even if they will end up sharing a lot of code together.

[jeremylenz]

the three workflows (library, version and repo exports) seem to be solving completely different user scenarios and the only thing they have in common is the fact that the end result is a "content export". Given the workflows are different (and so are the hammer commands: hammer content-export complete version - not hammer content-export complete --type version), it feels more natural (to me) to have different modules, even if they will end up sharing a lot of code together.

if the module catches this and tells me that I'm trying to do something that doesn't make any sense, I'll realize my error right away before any damage is done

Thanks for the thoughtful discussion everyone!

As a FAM newb I really like the idea of everything being explicit. So you each win one -- I like @evgeni's reasoning for having different modules for everything, and I like @wbclark's idea of passing explicit values instead of relying on hidden logic that may not be immediately apparent.

I'll move forward with this unless y'all feel super strongly and want to keep debating here.

[wbclark]

Thanks for the thoughtful discussion everyone!

As a FAM newb I really like the idea of everything being explicit. So you each win one -- I like @evgeni's reasoning for having different modules for everything, and I like @wbclark's idea of passing explicit values instead of relying on hidden logic that may not be immediately apparent.

I'll move forward with this unless y'all feel super strongly and want to keep debating here.

Actually, I will cede my point. I have been totally convinced by @evgeni 's perspective.

It makes the most sense not to have any incremental parameter at all, and just infer whether this is a complete or incremental export based on the presence of from_history_id.

[jeremylenz]

from_history_id:
    description:
      - Export history identifier used for incremental export. If not provided the most recent export history will be used.

Just realized that setting incremental based on the presence of from_history_id will remove the convenience described here - the user will have to look up a history ID for every incremental export. I think this tips the balance back in favor of explicit.. thoughts? @wbclark @parthaa @evgeni

[evgeni]

Oooh, if it can look up the recent automatically, then perfect, leave it!

[jeremylenz]

It might be valuable to show in the examples how to register that ID and save it for a future incremental export task

@wbclark I would love to, but I need help with how to do it :confused: (right now my IDs and content view names are hard-coded.)

I've tried using debugger: always but I didn't get far.

[jeremylenz]

okay, Added CV version and repository exports.

still need help with

• what's going on with the tests
• ~~how not to hard-code values in the test playbook~~

[evgeni]

Shall we sit down on Monday or so together and talk tests?

[jeremylenz]

Shall we sit down on Monday or so together and talk tests?

sounds good to me!

[jeremylenz]

Could you add maybe 2 examples beneath the DOCUMENTATION here, showing the full and incremental export?

@wbclark Done! :+1:

[jeremylenz]

Updated tests. Not sure what the failure means..

[evgeni]

Was the repository export added recently to Katello? The error says that there is no "repository" action on the exports controller.

As the tests run against a recorded api, it uses a static apidoc.json, which might be missing that (it's 4.3 right now, we're sometimes lazy updating)?

[jeremylenz]

Was the repository export added recently to Katello?

Yes, it's pretty recent..

[jeremylenz]

okay, if February is recent? https://github.com/Katello/katello/pull/9925

[evgeni]

https://github.com/Katello/katello/commit/4c1e72119659be50dd60682b1ef10e13c3ab682a seems to be in 4.5+, so yeah, "recent".

Feel free to replace katello.json with a fresh 4.5 one (but vanilla Katello with ostree enabled please, no other plugins)

[jeremylenz]

https://projects.theforeman.org/issues/34374 landed in Katello 4.5, so I guess that's the issue.

[jeremylenz]

@wbclark any pointers on how to fix the "checkmode" tests?

[jeremylenz]

:green_apple: :green_apple: :green_apple: :green_apple: :green_apple: :green_apple: :apple: :green_apple: :green_apple: :green_apple: :green_apple: :green_apple: :apple: :green_apple: :green_apple: :green_apple: :green_apple:

I believe the remaining test failures are unrelated!

[evgeni]

Here is the full diff I applied to the info module:

diff --git plugins/modules/content_export_info.py plugins/modules/content_export_info.py
index d9097595..6a17826a 100644
--- plugins/modules/content_export_info.py
+++ plugins/modules/content_export_info.py
@@ -34,26 +34,21 @@ options:
       - Export history identifier.
     required: false
     type: int
-  content_view_version_id:
+  content_view_version:
     description:
-      - Content view version identifier.
+      - Content view version.
     required: false
-    type: int
-  content_view_id:
+    type: str
+  content_view:
     description:
-      - Content view identifier.
+      - Content view name.
     required: false
-    type: int
+    type: str
   destination_server:
     description:
       - Destination server name
     required: false
     type: str
-  organization_id:
-    description:
-      - Organization identifier.
-    required: false
-    type: int
   type:
     description:
       - Specify complete or incremental exports.
@@ -62,14 +57,10 @@ options:
     choices:
     - complete
     - incremental
-  search:
-    description:
-      - Search string.
-    required: false
-    type: str
 extends_documentation_fragment:
   - theforeman.foreman.foreman
-  - theforeman.foreman.foreman.organization
+  - theforeman.foreman.foreman.katelloinfomodule
+  - theforeman.foreman.foreman.infomodulewithoutname
 '''
 
 EXAMPLES = '''
@@ -81,8 +72,6 @@ EXAMPLES = '''
     password: "changeme"
     server_url: "https://foreman.example.com"
 - name: "Get a specific export history and register the result for the next task"
-  vars:
-    organization_name: "Export Org"
   content_export_info:
     id: 29
     username: "admin"
@@ -92,13 +81,14 @@ EXAMPLES = '''
   register: result
 - name: "Write metadata.json to disk using data from the previous task"
   vars:
-    metadata: "{{ result['task']['results'][0]['metadata'] }}"
+    metadata: "{{ result['content_exports'][0]['metadata'] }}"
   ansible.builtin.copy:
     content: "{{ metadata }}"
     dest: ./metadata.json
 - name: "List all exports of a specific content view version"
   content_export_info:
-    content_view_version_id: 379
+    content_view: RHEL8
+    content_view_version: '1.0'
     username: "admin"
     password: "changeme"
     server_url: "https://foreman.example.com"
@@ -112,7 +102,7 @@ EXAMPLES = '''
     organization: "Default Organization"
 - name: "List incremental exports of a specific content view version marked for a specific destination server"
   content_export_info:
-    content_view_id: 1
+    content_view: RHEL8
     destination_server: "airgapped.example.com"
     type: incremental
     username: "admin"
@@ -121,7 +111,7 @@ EXAMPLES = '''
     organization: "Default Organization"
 - name: "List all exports of a specific content view marked for a specific destination server"
   content_export_info:
-    content_view_id: 1
+    content_view: RHEL8
     destination_server: "airgapped.example.com"
     username: "admin"
     password: "changeme"
@@ -130,35 +120,28 @@ EXAMPLES = '''
 
 '''
 
-from ansible_collections.theforeman.foreman.plugins.module_utils.foreman_helper import KatelloAnsibleModule, _flatten_entity
+from ansible_collections.theforeman.foreman.plugins.module_utils.foreman_helper import KatelloInfoAnsibleModule
 
 
-class KatelloContentExportInfoModule(KatelloAnsibleModule):
+class KatelloContentExportInfo(KatelloInfoAnsibleModule):
     pass
 
 
 def main():
-    module = KatelloContentExportInfoModule(
+    module = KatelloContentExportInfo(
         foreman_spec=dict(
             id=dict(required=False, type='int'),
-            content_view_version_id=dict(required=False, type='int'),
-            content_view_id=dict(required=False, type='int'),
+            content_view_version=dict(type='entity', scope=['content_view'], required=False),
+            content_view=dict(type='entity', scope=['organization'], required=False),
             destination_server=dict(required=False, type='str'),
-            organization_id=dict(required=False, type='int'),
             type=dict(required=False, type='str', choices=['complete', 'incremental']),
             search=dict(required=False, type='str'),
+            name=dict(invisible=True),
         ),
     )
 
     with module.api_connection():
-        module.auto_lookup_entities()
-
-        endpoint = 'content_exports'
-
-        payload = _flatten_entity(module.foreman_params, module.foreman_spec)
-        task = module.resource_action(endpoint, 'index', payload)
-
-        module.exit_json(task=task)
+        module.run()
 
 
 if __name__ == '__main__':

[evgeni]

I am happy with the content_export_library module, but I think the others need a bit more work to eliminate the cv_id etc references.

Shall we split this PR into four, one per module? Then we can get the library one in instantly and work on the others?

[jeremylenz]

@evgeni I need to keep info and library together because the tests use info..

[jeremylenz]

See https://github.com/theforeman/foreman-ansible-modules/pull/1453 and https://github.com/theforeman/foreman-ansible-modules/pull/1454

I'll rebase them once this is in.

[jeremylenz]

also I don't know how to make lint happy

[evgeni]

the remaining sanity errors will be fixed in https://github.com/theforeman/foreman-ansible-modules/pull/1455 and are unrelated to this PR!