port foreman_domain, foreman_search_facts and katello_content_credential to apypie #261

evgeni · 2019-04-02T18:24:55Z

Some time ago, I've written apypie as a Python port of apipie-bindings. This has the benefit that it can fetch the API definition from the running Foreman instance and thus does not need to "know" about the various resources. It's far from feature complete (or even bug-free), but I thought it never will become if it has no users, so here we are.

This PR ports foreman_domain to apypie instead of nailgun. While doing so, I've tried to make the ForemanAnsibleModule API more Pythonic. The code also contains a few FIXMEs, which I'm still unsure about how to handle cleanly. Also, the method/variable names are inconsistent, as I've copied a lot from the nailgun helper, but only adapted the naming half way through.

evgeni · 2019-04-02T18:26:41Z

@akofink @sean797 @mdellweg @manuelbonk @Fobhep @ehelms I want y'all thoughts

ekohl · 2019-04-02T18:33:38Z

modules/foreman_domain.py

@@ -87,36 +86,27 @@
 RETURN = ''' # '''

 try:
-    from ansible.module_utils.ansible_nailgun_cement import (
-        find_domain,
-        find_locations,


This, orgs and proxies do make sense to me to keep since they often show up in APIs. Perhaps there needs to a compatible apypie helper?

I thought about that (even had them at first), but then I thought that find_foo(…) is not that shorter than find_resource_by_name('foo', …) and having that for only a few resources is also weird?

There is still the thin=True as well. Perhaps the normalization function should be factored out and do it there.

ekohl · 2019-04-02T18:34:33Z

test/test_playbooks/fixtures/domain-3.yml

    method: GET
-    uri: https://foreman.example.com/katello/api/v2/ping
+    uri: https://192.168.121.223/api/status


I quite liked the foreman.example.com domain :)

blah :)
I'll fix the fixtures at some point, yeah.

module_utils/foreman_helper.py

sean797

Super awesome @evgeni I was wondering what happened with apypie only yesterday. really nice

module_utils/foreman_helper.py

ekohl · 2019-04-05T09:08:45Z

module_utils/foreman_helper.py

+
+
+def sanitize_entity_dict(entity_dict, name_map):
+    return {value: entity_dict[key] for key, value in name_map.items() if key in entity_dict}


How about:

Suggested change

return {value: entity_dict[key] for key, value in name_map.items() if key in entity_dict}

result = {}

for (key, (value, normalization)) in name_map.items():

if key in entity_dict:

if normalization:

result[value] = module.find_resources(normalization, entity_dict[key], thin=True)

else:

result[value]: entity_dict[key]

return result

Though it doesn't deal with the by name/id problem, it does allow dropping a lot of code from individual modules. Perhaps the mapping needs to be a bit more expressive though.

sanitize_entity_dict is something we also use in the nailgun based modules, so I'd prefer not to touch it just yet. but I agree, there is certainly room for further improvements

evgeni · 2019-04-05T11:13:23Z

This now also includes an example for Katello 💃

sean797 · 2019-04-05T11:24:14Z

module_utils/foreman_helper.py

@@ -120,9 +124,13 @@ def find_resource(self, resource, search, failsafe=False, thin=False):
                result = self.show_resource(resource, result['id'])
        return result

-    def find_resource_by_name(self, resource, name, failsafe=False, thin=False):
+    def find_resource_by_name(self, resource, name, organization_id=None, failsafe=False, thin=False):


We may want to allow passing in other params, for exmaple https://theforeman.org/plugins/katello/3.9/api/apidoc/v2/repository_sets/index.html requires product_id, I'm fairly sure there are others.

But I think we can change this at a later date when/if we require them.

Yea, maybe this could be done with a dict parameter or with kwargs.

Yeah, this is something I should change before merging!
Otherwise fixing the API later is pain.

updated to be a dict.

mdellweg · 2019-04-15T11:25:24Z

Is this something that can be extended easily to be using APIv3 (Graphql)?

evgeni · 2019-04-15T12:52:16Z

Is this something that can be extended easily to be using APIv3 (Graphql)?

I don't know much about the GraphQL API, but I don't think this will be easy.

evgeni · 2019-04-17T07:49:20Z

Is this something that can be extended easily to be using APIv3 (Graphql)?

I don't know much about the GraphQL API, but I don't think this will be easy.

After looking a bit closer on how GraphQL works, this will not work with our GraphQL API as such, but we should be able to create a similar groundwork for APIv3 with learnings from this and previous PRs.

evgeni · 2019-04-18T07:43:42Z

I think this is now at a state where we should talk about: do we want that as a direction forward, or was this a nice PoC but we stick to nailgun.

Let me re-iterate the pros and cons for the switch:

Pros

apypie has only one dependency: requests, so shipping it to users (via RPM or whatever) is much easier than nailgun which pulls in multiple Python libs (which are not packaged in Fedora/EPEL).
We're using the API JSON as published by the server, thus always having the matching list of entities and their params. No need to update the library if a new entity is added or changed.
This also means we don't really need to branch per Foreman/Katello release (although Hammer does it, so there might be cases where we still have to).

Cons

apypie is a young library, which didn't see much adoption yet, so there will be bugs and dragons
we're losing typechecking of params, as these are not exposed in the API JSON and apypie has no further knowledge about them
we're losing relationships between entities, so the modules will need to potentially pull more data themself.

Known Bugs

You currently have to wipe your ~/.cache/apypie/ before recording VCR tapes, as otherwise apypie takes the API JSON from the cache and it's not recorded, resulting in CI failures where there is no cache.

sean797 · 2019-04-18T14:43:37Z

Cons

we're losing typechecking of params, as these are not exposed in the API JSON and apypie has no further knowledge about them

we're losing relationships between entities, so the modules will need to potentially pull more data themself.

Could we extend apipie and by extension apypie to know about these? I imagine that would take a farily long time to do, but it would be awesome if Foreman and its plugins had a 100% self docuementing API

ekohl · 2019-04-18T14:47:37Z

Rather than extending apipie's custom format, I'd rather invest in moving to the OpenAPI spec. apipie can already output that, but it's likely we need to enhance some things for better output.

mdellweg · 2019-04-26T14:02:04Z

Is there any heuristic to check for an outdated apypie cache?
What happens if you fuel two foremans with different versions in the same playbook?

evgeni · 2019-04-26T15:25:10Z

@mdellweg yes. The cache is url based, so it will not clash. And apypie checks the last modified header too.

evgeni · 2019-04-30T06:07:30Z

Given all the emoji reactions to the initial PR comment, I'd think y'all are in favor of this, but can you express that a bit more explicitly please? :)

mdellweg · 2019-04-30T10:47:59Z

I know we had the discussion about using nailgun. So the question that comes to my mind is: Will this be adopted as a successor of nailgun and also be used in robottelo?
Thinking only about foreman-ansible-modules, i like the approach.

akofink · 2019-04-30T13:59:19Z

The way I see it, QE is, and operates like, a different team from the Katello/Foreman developers. There's a set of projects for using with Foreman in production (plugins, cli, ansible playbooks, etc.) which Foreman devs work on and a set of repositories used to test Satellite by QE (nailgun, robottelo, airgun, etc.) that QEs work on. This is the problem with Nailgun IMO - it's a QE utility, not a tested and vetted production-use Foreman API python library. The GitHub org which these utilities live is SatelliteQE - clearly not intended for upstream use.

A secondary related issue with Nailgun is that it's "release cycle" (if you'd call it that) is tied to Satellite, not Foreman. We'd need buy in from QE to uproot the status quo and start tracking upstream releases, which happen 3X more frequently than downstream. If you didn't already guess, they are already very busy.

Lastly, there's some packaging issue with Nailgun that has been previously mentioned due to one of its dependencies (python requests? idk).

So, yes, 🚀 for a python library that is automatically built from apipie. When something changes upstream, there's no code to write or update, just an API doc to generate (and tests to re-run).

ekohl · 2019-04-30T15:19:50Z

Nailgun is, as I understand it, much more focused at verifying the API. This library is intended to just consume what's there. Perhaps Nailgun could even use apypie underneath, but that verification still needs to happen.

ehelms · 2019-05-10T12:12:56Z

Generally 👍 to this approach as it also feels like it will make cross-version usage more robust.

How much work do you estimate it is to convert all entities?

mdellweg · 2019-05-10T13:46:46Z

I actually think, entities can be migrated gradually. We can track this process in a new checklist issue.

evgeni · 2019-05-13T13:10:14Z

Ad @mdellweg's question: nailgun is an API client that has it's primary usecase in testing said API. As such it cannot rely on the API to be correct, but needs to "know" endpoints, entities and data structures itself. As such, apypie is not a replacement for nailgun in all use-cases and I think robottelo is one of these who will remain on nailgun.

The packaging issue @akofink mentioned is not about requests but blinker_herald and inflection, as these are not packaged today. It's not that much of an issue as an nuisance, as noone wants to maintain more packages than absolutely required.

Ad @ehelms question: As you can see from this PR, the diff for the actual module is quite small, but you'll need to run the tests against a live system a few times to get all the details right and I wouldn't be surprised if we find a few bugs while porting the first entities. Overall the amount of work is somehwere in the 30-60 minutes per entity, so not that much. And as @mdellweg wrote, this can (and should) happen gradually. (And I'd not convert all of them, just the ones covered by tests ;))

mdellweg · 2019-05-13T14:00:20Z

(And I'd not convert all of them, just the ones covered by tests ;))

...which is the vast majority:
#77

(also i don't want to stop promoting this issue ;))

ehelms · 2019-05-14T15:26:13Z

I think then my ask would be to snapshot the current pure nailgun either via establishing a version and thus release for the modules or simply branching the current state and then merging this with an eye towards a release milestone that is fully converted to apypie.

mdellweg · 2019-05-15T07:21:16Z

I have one last concern before we can actively start to bring this forward: How would the fact module look like? I think it is the one that is fundamentally different and we should at least have something equivalent.

evgeni · 2019-05-15T14:06:50Z

@mdellweg that's a fair ask, I'll prep the facts module too, so you'll have an example :)

evgeni · 2019-05-15T15:40:59Z

@mdellweg there we go, I still need to re-record the VCR tapes, but it's working.

please note, this "breaks" the api, as now we have to write settings instead of Setting (nailgun uses singular names, where as Foreman does not)

mdellweg · 2019-05-15T16:12:22Z

modules/foreman_search_facts.py



 def main():

-    module = ForemanAnsibleModule(
+    module = ForemanApypieAnsibleModule(


Is this name going to stick, once we ridded nailgun?

As soon as we're back to one backend, we can switch the naming back. I don't mind either way.

mdellweg · 2019-05-15T16:24:54Z

please note, this "breaks" the api, as now we have to write settings instead of Setting (nailgun uses singular names, where as Foreman does not)

I think i am ok with this. It is an adequate replacement. Thanks for taking that concern seriously.

evgeni · 2019-05-16T11:45:48Z

test/test_playbooks/search_facts.yml

      return_length: 0
-  - name: Run with invalid resource


this is currently not supported with apypie and will need some love there.

For the sake of clarity, how will users know what resources are valid? I assume they won't today and we can address that once we have made the required changes in apypie?

Correct. My plan is to report back the available resources in the error message from apypie when a non-available one is called, and then we can restore the code we had.

evgeni · 2019-05-16T12:29:47Z

@ehelms I've created a nailgun branch which is at the same level as current master: https://github.com/theforeman/foreman-ansible-modules/tree/nailgun

sean797 · 2019-05-16T13:32:22Z

please note, this "breaks" the api, as now we have to write settings instead of Setting (nailgun uses singular names, where as Foreman does not)

I too am also fine with that, considering we are doing a new major release.

With this PR, could you add something prominent to the README so users know about that branch?

evgeni · 2019-05-17T08:47:46Z

@sean797 done

mdellweg · 2019-05-20T14:07:27Z

module_utils/foreman_helper.py

+        changed, _ = self.ensure_resource(resource, entity_dict, entity, state, check_missing, check_type, force_update)
+        return changed
+
+    def ensure_resource(self, resource, entity_dict, entity, state, check_missing=None, check_type=None, force_update=None):


I can only speculate, you didn't like the name naildown_entity enough...

I just didn't want the problem to look like a nail ;)

evgeni · 2019-05-20T15:51:17Z

this should pass tests (again) and has a more or less clean history

so will merge tomorrow when back in the office :)

ekohl reviewed Apr 2, 2019

View reviewed changes

sean797 reviewed Apr 2, 2019

View reviewed changes

module_utils/foreman_helper.py Outdated Show resolved Hide resolved

module_utils/foreman_helper.py Show resolved Hide resolved

module_utils/foreman_helper.py Outdated Show resolved Hide resolved

sean797 reviewed Apr 3, 2019

View reviewed changes

module_utils/foreman_helper.py Show resolved Hide resolved

evgeni commented Apr 5, 2019

View reviewed changes

module_utils/foreman_helper.py Show resolved Hide resolved

ekohl reviewed Apr 5, 2019

View reviewed changes

evgeni force-pushed the apypie branch from c03b76a to b701cce Compare April 5, 2019 11:07

sean797 reviewed Apr 5, 2019

View reviewed changes

evgeni force-pushed the apypie branch from b701cce to f42a732 Compare April 18, 2019 07:28

evgeni marked this pull request as ready for review April 30, 2019 06:05

mdellweg reviewed May 15, 2019

View reviewed changes

evgeni commented May 16, 2019

View reviewed changes

evgeni mentioned this pull request May 16, 2019

implement useful error messages Apipie/apypie#19

Closed

ehelms approved these changes May 20, 2019

View reviewed changes

mdellweg approved these changes May 20, 2019

View reviewed changes

evgeni force-pushed the apypie branch 2 times, most recently from a03fbcd to c7694dd Compare May 20, 2019 15:43

evgeni added 4 commits May 20, 2019 17:46

port foreman_domain to apypie

b0d73de

port katello_content_credential to apypie

1badd4c

port foreman_search_facts to apypie

34d96d8

document branches

7ba60ee

evgeni force-pushed the apypie branch from c7694dd to 7ba60ee Compare May 20, 2019 15:46

evgeni changed the title ~~[RFC] try out using apypie instead of nailgun~~ port foreman_domain, foreman_search_facts and katello_content_credential to apypie May 21, 2019

evgeni merged commit c45f41c into theforeman:master May 21, 2019

evgeni deleted the apypie branch May 21, 2019 08:40

evgeni mentioned this pull request May 23, 2019

Require git version of nailgun #245

Closed



		def sanitize_entity_dict(entity_dict, name_map):
		return {value: entity_dict[key] for key, value in name_map.items() if key in entity_dict}

-    return {value: entity_dict[key] for key, value in name_map.items() if key in entity_dict}
+    result = {}
+    for (key, (value, normalization)) in name_map.items():
+        if key in entity_dict:
+            if normalization:
+                result[value] = module.find_resources(normalization, entity_dict[key], thin=True)
+            else:
+                result[value]: entity_dict[key]
+    return result

port foreman_domain, foreman_search_facts and katello_content_credential to apypie #261

port foreman_domain, foreman_search_facts and katello_content_credential to apypie #261

Conversation

evgeni commented Apr 2, 2019

evgeni commented Apr 2, 2019

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

sean797 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni commented Apr 5, 2019

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

mdellweg commented Apr 15, 2019

evgeni commented Apr 15, 2019

evgeni commented Apr 17, 2019

evgeni commented Apr 18, 2019

Pros

Cons

Known Bugs

sean797 commented Apr 18, 2019

Cons

ekohl commented Apr 18, 2019

mdellweg commented Apr 26, 2019

evgeni commented Apr 26, 2019

evgeni commented Apr 30, 2019

mdellweg commented Apr 30, 2019

akofink commented Apr 30, 2019

ekohl commented Apr 30, 2019

ehelms commented May 10, 2019

mdellweg commented May 10, 2019

evgeni commented May 13, 2019

mdellweg commented May 13, 2019

ehelms commented May 14, 2019

mdellweg commented May 15, 2019

evgeni commented May 15, 2019

evgeni commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdellweg commented May 15, 2019 • edited Loading

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

evgeni commented May 16, 2019

sean797 commented May 16, 2019

evgeni commented May 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni commented May 20, 2019

mdellweg commented May 15, 2019 •

edited

Loading