Resolve incorrect error message when creating duplicate item without required field.

[jangsutsr]

Connect to issue #109.

This issue is caused by the exe sequence of existence checking and required checking. Adding related exception capture gives user log info of possible failure causes.

Now the outcome becomes:

(tower_cli_devel) sitan-OSX:tower-cli sitan$ tower-cli host create --name localhost -v --insecure
*** DETAILS: Checking for an existing record. *********************************
GET https://ec2-54-234-179-218.compute-1.amazonaws.com/api/v1/hosts/
Params: {'name': u'localhost'}

*** WARNING: Multiple Results found. Possibly caused by missing required options. 
Error: Missing required fields: inventory

[coveralls]

Coverage decreased (-0.1%) to 99.866% when pulling cf35913 on jangsutsr:issue109 into 86cf8e0 on ansible:master.

[coveralls]

Coverage decreased (-0.1%) to 99.866% when pulling cf35913 on jangsutsr:issue109 into 86cf8e0 on ansible:master.

[coveralls]

Coverage remained the same at 100.0% when pulling f3f1106 on jangsutsr:issue109 into 86cf8e0 on ansible:master.

[AlanCoding]

Nice job nailing down the source of this error! I see exactly what you're getting to, although I can't convince myself that the normal error for multiple results will still work as intended. We want that error "Error: Missing required fields: inventory" to be thrown, and I think your test is good for this.

Here's what I would offer as a small adjustment to what you did:

Take these lines:

https://github.com/jangsutsr/tower-cli/blob/f3f110626e3b191f53800e6772946b7e7e98dccf/lib/tower_cli/models/base.py#L570-L571

Move them to before the call to _lookup. Since this doesn't need existing_data or pk, this is a good option. That means that you will have missing_fields in the off-chance that you get into your except block.

With that, you can integrate this error message into your except block:

https://github.com/jangsutsr/tower-cli/blob/f3f110626e3b191f53800e6772946b7e7e98dccf/lib/tower_cli/models/base.py#L572-L574

Depending on if existing_data has any content, you could raise an accurate error message.

There are other ways to do this too, and I could be wrong about the existing multiple results scenario too.

[jangsutsr]

@AlanCoding Nice catch! Never a good idea to mute an exception which is meant to be thrown. After studying I decided to restructuring rather than simply adding exception catch. In my opinion, the _lookup of write function will only be called when creating rather than modifying resources (modification happens only when pk is provided). Therefore it is reasonable to put duplication check right before _lookup, since required attribute is only useful during creation.

Now the outcome becomes:

(tower_cli_devel) sitan-OSX:tower-cli sitan$ tower-cli host create --name localhost -v --insecure
Error: Missing required fields: inventory

And

(tower_cli_devel) sitan-OSX:tower-cli sitan$ tower-cli host create --name localhost --inventory 1 -v --insecure
*** DETAILS: Checking for an existing record. *********************************
GET https://ec2-54-234-179-218.compute-1.amazonaws.com/api/v1/hosts/
Params: {'inventory': 1, 'name': u'localhost'}

*** DECISION: Record already exists, and --force-on-exists is off; do nothing. 

== ========= ========= ======= 
id   name    inventory enabled 
== ========= ========= ======= 
 1 localhost         1    true
== ========= ========= ======= 

Note some unit tests were also modified.

[coveralls]

Coverage decreased (-0.8%) to 99.175% when pulling ec7f606 on jangsutsr:issue109 into 86cf8e0 on ansible:master.

[AlanCoding]

Part of the problem here is that pk could be defined after the subsequent to _lookup is done, and that's part of what was originally being tested for (I think, and I think that's tied up with the test changes you were forced to make). I would have the missing_fields calc happen even before the if not pk: block, and then maybe use a conditional along with fail_on_found to raise another type of error of that happens (this could still be in the original except block you used). FOF is a good indicator that this is called by the create command.

[jangsutsr]

It turns out that the very desired exception is not able to be raised in this scenario because the logic is locked: one can only run the lookup to determine whether to create or modify resource, yet the required field constraint is only valid when creating resource.

A walk-around is to make the error message more informative, so that users can more easily spot the cause of error.

[AlanCoding]

👍 This looks fine. Let's go ahead and merge this and close the ticket out.

[coveralls]

Coverage remained the same at 99.175% when pulling 02b8015 on jangsutsr:issue109 into bb0e52e on ansible:master.