Porting Collections Off of Tower CLI

[beeankha]

SUMMARY

The related AWX issue can be found here: #5145. This is a collaborative PR between myself and @john-westcott-iv .

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

• Collections

[AlanCoding]

link #5474

[squidboylan]

I left an inline comment about tower_cli.cfg default paths, another thing to note is that the tower_cli.cfg supports both yaml and key=value format, which we should make a decision on whether we need/want to support.

[AlanCoding]

Before this gets too far into the weeds, I want to ask about the highest priority design item, which is the consistency of how these methods are called.

It would be really good to distinguish all methods that terminate execution - that is they do exit_json or fail_json. I would also like to be very careful that they either always terminate or never terminate. You've done an excellent job fitting a solution to do that with the on_changed callback. Whether they terminate or not is so important, I almost think it should be in the method name.

On to the call arguments, the core CRUD actions involve update_if_needed, post_endpoint, and delete_endpoint. Consider the arguments for post_endpoint versus this method here.

def post_endpoint(self, endpoint, handle_return=True, item_type='item', item_name='', *args, **kwargs):

This just doesn't look the same at all. My position is that these should all have a common set of functionality - because they are terminating methods.

Likewise, for deletion, you handle the case of changed=False in the module code very elegantly, but that's not consistent with this. If we have a method update_if_needed, then we should have a delete_if_needed method so that we know that (a) it will terminate execution and (b) it could terminate with multiple types of statuses.

I come back around to the design that tower-cli used, maybe we should have a single method for CRU and another one for D. Since the module itself deals with the state parameter, this makes a lot of sense with the surface area that Ansible requires.

[john-westcott-iv]

Great feed back.

In thinking about, I'm learning towards having C, U, and D methods. In the form of:
update_if_needed
create_if_needed
delete_if_needed

I don't want to put _and_exit on the functions since you can override the exit with handle_response=False and my gut is saying we will find some use case where we might have a "compound" action we need to take. I.e. create item 'a' and then create item 'b' which has a reference to 'a'. So the first one would handle_exit=False and the second one is True.

Each of the methods above can have a corresponding hook for on_create, on_update, and on_delete. Do you think those should be "registered" like I did or just make them parameters to the if_needed methods?

The R will continue to be handled via get_one and get_all (I'm thinking we can rename get_all_endpoint to just get_all for consistency).

In addition, I think it would make sense to merged CU method called create_or_update_if_needed so that modules can get into a very simple pattern like:

if state == 'absent':
    delete_if_needed(<args>)
else:
    create_or_update_if_needed(<args>)

Another other option would be to make a single method like: put_object_in_state(old_object, new_object, state, ...) which would handle all of the CUD operations in one. Any thoughts on that? That would put even the conditional logic into the module_util so the modules would only need to:

• resolve associated IDs
• build the object data structures
• define any on_* methods
• make the put_object_in_state call.

[AlanCoding]

I do feel partial to the CU, and D methods, exactly how your proposed code snippet is. But as long as we have consistent args with calls to _if_needed methods, then I give my 👍

my gut is saying we will find some use case where we might have a "compound" action we need to take. I.e. create item 'a' and then create item 'b' which has a reference to 'a'. So the first one would handle_exit=False and the second one is True.

Good point, we should be prepared to complex multi-action scenarios. Given that, handle_exit=False sounds like a good solution to me... probably better than the callback approach. If the actions got complex, then we would wind up in callback hell. The way tower_project waits for update could be re-written to use a handle_exit=False kwarg and not the callback.

[john-westcott-iv]

Check it out now. I left the option for a callback for "simple" things, like waiting for a project update to complete. For more complex things you can give it handle_response=False but then you are on the hook for processing the return and follow up actions.

[AlanCoding]

I'm on board with the call pattern in the modules now. Having both the callback and handle_response kwarg has a pretty good justification.

[AlanCoding]

As a specific example, following up to my last comment, both delete_endpoint and post_endpoint need the plural name used in the list endpoint, the string "teams" here. The method update_if_needed avoids this by using the URL in the data, but it doesn't need to. It could also do a PATCH to 'teams/{0}'.format(team['id']), and I would prefer this because it's more consistent.

The arg item_name doesn't seem to me like something we need to keep. We could default to looking for name, and then have some other means of overriding that on initialization of the class for the special cases.

The data is clearly needed in all cases.

So this points us in the direction of a call pattern like 'teams', team_fields, and you could share that pattern between all the methods from module_utils.

[AlanCoding]

This looks great! I can't wait to look into check_mode and diff_mode later on. That will be so painless by comparison.

[AlanCoding]

Doesn't this need to pass in the plural endpoint? So I would expect that create_or_update_if_needed will also need to take the plural endpoint.

Alternatively, it would make sense to me to pass that into __init__. It's a general property of a resource, as long as we're only declaring it once for each module, that's about the most efficient solution we could hope for. You could even take it in optionally, and just add an "s" if it's not given.

[john-westcott-iv]

You are correct, this will need the endpoint passed into it and then on down to create_if_needed.

The init option would be cool but if we are trying to keep complex use cases in mind that would require an override option anyway.

Finally for pluralization, I wish we could auto-pluralize a friendly name but inventorys is not valid also; if anyone ever wanted to hit /me, /config, ping, etc auto-pluralization logic could become tricky and something that would need to be maintained. It feels like it might almost work better to do an un-pluralization to derive the friendly name from the endpoint.

But I'll go ahead and get the pass through of the endpoint working and then touchup the converted modules to use the new syntax; thanks for the review.

[AlanCoding]

I don't like that we're still doing this, but I don't want to add one more deprecated thing into this PR.

But I might make an issue to get rid of all these instances, and wanted to ask how you would feel about that. In order to optionally take the "@" character, that means the parameter type is string, and I would much prefer that the type be a dict.

[john-westcott-iv]

So I almost said I 100% agree on this but then I started to think about it....
The only reason this notation was supported was because it had to be fed into Tower CLI that way. Since we are deprecating Tower CLI with this PR one could argue that now is the perfect time to drop this. If we wanted to do that we could provide in the release notes something like:

IF you used the @ method to import a vars file, here is how you would convert your code to have Ansible load a vars file.... one of the caveats would be that the @ syntax as is loads the file from the targeted node where as using a lookup plugin would load from the controller.

Also, as an FYI, in my test I passed in something like:
variables:
foo: bar
Ansible generates a warning that its forcing a dict into a str but other than that it works.

[AlanCoding]

IF you used the @ method to import a vars file, here is how you would convert your code to have Ansible load a vars file....

probably similar to:

https://github.com/ansible/awx/blob/devel/awx_collection/plugins/modules/tower_job_template.py#L242

I'm cautiously optimistic that no one will have problems with these parameter type changes - because the change was prompted by Ansible core itself. Their CI started enforcing types, but old modules were given an exception. The variables param here technically passes their CI, but doesn't respect the spirit of enforcing types. So at minimum, we have someone else to point the finger at.

[beeankha]

We can go ahead and take this out (changes currently in progress). Do we want to raise an exception or warn that the "@" syntax is no longer supported?

[AlanCoding]

Won't the API itself give a 400 in this event? If it does that, and if the 400 error is handled correctly, then I don't think we should need this.

that case can be unit tested. As long as it gives a sensible error message that says mostly the same thing, we shouldn't need this conditional.

[beeankha]

The API returns a 405 without a helpful error message:

I filed a bug for this here: #5744

[softwarefactory-project-zuul]

Build succeeded.

• awx-api-lint : SUCCESS in 17m 02s
• awx-api : SUCCESS in 14m 28s
• awx-ui : SUCCESS in 11m 57s
• awx-ui-next : SUCCESS in 8m 55s
• awx-swagger : SUCCESS in 14m 03s
• awx-detect-schema-change : SUCCESS in 18m 38s (non-voting)
• awx-ansible-modules : SUCCESS in 5m 36s

[softwarefactory-project-zuul]

Build succeeded (gate pipeline).

• awx-api-lint : SUCCESS in 4m 47s
• awx-api : SUCCESS in 10m 35s
• awx-ui : SUCCESS in 9m 43s
• awx-ui-next : SUCCESS in 13m 04s
• awx-swagger : SUCCESS in 10m 03s
• awx-detect-schema-change : SUCCESS in 13m 58s (non-voting)
• awx-ansible-modules : SUCCESS in 4m 52s
• awx-push-new-schema : SUCCESS in 11m 22s (non-voting)