Deal with ansible not having a username

[jeffwarnica]

Per https://bugzilla.redhat.com/show_bug.cgi?id=1621885

[agrare]

@jeffwarnica would it be better to check if userid is nil before assuming it is present rather than set it to a "random" string? Doing this turns a simple unless userid.nil? into a string compare, and actually sending "<no username provided>" to a provider is probably not what we want.

[jeffwarnica]

If find_or_build() found something, then we are updating a record. The LHS, if once a string, is invalid; "" is more correct then the username that someone removed from Tower.

|| nil might also be OK, but since I've hit an unrelated UI problems displaying nil today, caution, in giving it something, seems reasonable here.

[agrare]

If find_or_build() found something, then we are updating a record.

Mmm that's not quite right, find_or_build looks in the inventory_collections not the database so it could "find" an inventory object that was created by another collection's parser. Either way I don't think it is relevant.

The LHS, if once a string, is invalid "" is more correct then the username that someone removed from Tower.

If someone removed a username from tower and credential.username == nil then isn't setting inventory_object.userid = nil correct?

|| nil might also be OK

|| nil isn't needed, if credential.username || "<...>" would work then credential.username is already nil and just inventory_object.userid = credential.username would work.

but since I've hit an unrelated UI problems displaying nil today, caution, in giving it something, seems reasonable here.

Do you have a traceback from where the userid being nil caused an error? It is probably not hard to fix that. If we have issues with this then we might need a data migration to update any nil userid's to some other string.

Unless we have presence validations on the model (we don't) then nil here should be a valid value.

[agrare]

Just noticed you added a BZ, looking at the logs the error is:

ERROR -- : [NoMethodError]: undefined method `username' for #<AnsibleTowerClient::Credential:0x0000000009ba3098>  Method:[block in method_missing]
ERROR -- : /opt/rh/cfme-gemset/bundler/gems/cfme-providers-ansible_tower-9841e4e2793c/app/models/manageiq/providers/ansible_tower/shared/inventory/parser/automation_manager.rb:101:in `block in credentials'

Which means the issue isn't that credential.username is nil, it means that credential doesn't even respond_to username so you would need something like inventory_object.userid = credential.username if credential.respond_to?(:username)

[jeffwarnica]

My solution does work, but I'll defer to any better rubyist around if there is a more correct solution.

Doesn't guard against something that wants inventory_object.userid to be non-nill later, though.

Which ever you want.

[agrare]

My solution does work

That's interesting, I tried that out and I get the same error:

>> AnsibleTowerClient::Credential.new('/api', {}).username || "no credentails"
NoMethodError: undefined method `username' for <AnsibleTowerClient::Credential >:AnsibleTowerClient::Credential

which is expected because || doesn't handle a NoMethodError exception earlier in the statement.

[agrare]

@jameswnl can you take a look?

[jameswnl]

This seems is caused by ansible/ansible_tower_client_ruby#95 and ansible/ansible_tower_client_ruby#68.
It has to do with the way Anisble Tower Gem's classes are dynamically defined. If there hasn't been any Credential that has a field of username, the class wouldn't have that method at all.

[jameswnl]

You can do inventory_object.userid = credential.try(:username)

And for the UI issue that you mention, let us know what exactly that is and it's likely better to be done in UI side.

[jeffwarnica]

I'm just theorizing that the UI may barf if .userid is null. (It has in a totally unrelated case I ran into last week).

[agrare]

IMO if it fails we should fix it given that this is a valid case for this provider, @jameswnl can you test this on the ansible provider screens? I tested a nil userid with vmware credentials and it didn't blow up.

[jameswnl]

I tested external Tower provider case and here're what I found:

1. There's no CFME UI showing the credentials
2. The userid is "" (empty string) for a credential that has no username defined.

So the following would work
inventory_object.userid = credential.try(:username) || ""

(I didn't hit the exception because of the issue in Ansible Tower gem)

[agrare]

I still think || "" is a bad idea, this is assuming that we have an issue with nils before we actually have one. If username cannot be nil we need to add a validation to the model, not sprinkle these all over the place

[jeffwarnica]

Where might that be, @agrare ? Even with rubymine, navigating around this code is an adventure.

[agrare]

Do you mean where is the model?

[jameswnl]

I agree that we can let the nil to go in and fix wherever that may fail.

[jeffwarnica]

Yeah, where is the model?

[agrare]

https://github.com/ManageIQ/manageiq/blob/master/app/models/auth_userid_password.rb

But again we shouldn't force it to be non nil unless we actually need to

[Ladas]

The validation would not really help anyway, since that would just fail and we would need to do credential.try(:username) || "" or set default value.

Looking at the BZ, it should be enough to have inventory_object.userid = credential.try(:username)

[miq-bot]

Checked commits jeffwarnica/manageiq-providers-ansible_tower@769239e~...34edb90 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪