Tower 3.3 fix: Credential.manager_ref need to be an integer for Tower 3.3

[jameswnl]

Part of the fix to https://bugzilla.redhat.com/show_bug.cgi?id=1640533

Tower 3.3 changed validation rule that now the type of reference to credentials (and others) must be an integer.

Automate code would make use of this new method

Related PR: ManageIQ/manageiq#18154

[miq-bot]

Checked commit https://github.com/jameswnl/manageiq-providers-ansible_tower/commit/14b5e83174492aa869a2b15976fc59f3bb9afc36 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

[gmcculloug]

One question about the potential handling of a nil value, but otherwise I think this looks good.

@agrare Can you review?

[gmcculloug]

We have tested with this change in a few scenarios and it resolves the reported issue. 👍

@jameswnl Is there any condition you can think of where this column could end up as nil and we may not want to convert that to an int?

[jameswnl]

nil.to_i returns 0 and also this is implemented in Tower subclass.

But talked to @agrare has concern on changing the type returned. So your team may have to explore performing the conversion in automate code

[gmcculloug]

We would have to find all the places where we do something like: https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/embedded_ansible/automation_manager/playbook.rb#L49 (There are other places that do something similar.)

Maybe the other third option is to do this at the client gem layer so the database and code can continue the same but the client knows what Tower is expecting.

[jameswnl]

@gmcculloug going Tower client gem route, you mean updating https://github.com/ansible/ansible_tower_client_ruby/blob/master/lib/ansible_tower_client/base_models/job_template.rb#L12 to search/replace the options param?
hmm.. I am not sure if this is a clean way ...

[agrare]

So I have a few concerns, might turn out that none of them are a big issue but:

1. There will be inconsistent results depending on how you ask
   Authentication.pluck(:manager_ref) <- will return all strings
   Authentication.all.map { |a| a.manager_ref} <- will return a mix of strings and ints
   ManageIQ::Providers::AnsibleTower::AutomationManager::Credential.pluck(:manager_ref) <- will return all strings
   ManageIQ::Providers::AnsibleTower::AutomationManager::Credential.all.map { |a| a.manager_ref} <- will return all ints

This just seems like a recipe for someone doing ems.authentications.map { |auth| auth.manager_ref.split("something") } or some string op and it'll blow up when it gets an int

2. If there isn't something which is castable to an int in the manager_ref field it will always return 0
   0 Could be a valid credential that isn't the one the user selected.

We could use Integer(manager_ref) so that it would raise an exception? IDK

So high level I don't see anything explicitly wrong, just strange

[agrare]

super.to_i would be cleaner

[jameswnl]

I don't have a preference but the self[:attr] is based on https://stackoverflow.com/questions/21835116/how-can-i-overwrite-a-getter-method-in-an-activerecord-model

[gmcculloug]

If we want to use something like manager_ref_obj can that be a virtual column? This would still requires us to modify all the callers.

[jameswnl]

@bdunne overriding manager_ref which was my initial approach. See @agrare 's concern in this thread

[bdunne]

I see. I guess my concern is that all callers need to be updated and new code needs to know about the new method.

[jameswnl]

yeah, that's the drawback of creating a new method.

[gmcculloug]

We discussed a couple of approaches and this felt like the best options.

One was overriding manager_ref for this sub-class to return an integer, but that would be if you loop over all instances the same property would return different object types. Some integer some string.

Also, discussed changing it down at the tower client gem, but that was James/Adam were not in favor of that either.

Key point, I'm hoping to get this resolve for the next Hammer build and Lucy is working on the corresponding changes so if we think this is going to change then let's get it addressed now. Otherwise I'd like to move forward with this approach.

[jameswnl]

@miq-bot assign @agrare

[jameswnl]

@agrare can we merge this?

[simaishi]

Hammer backport details:

$ git log -1
commit a45325609bd2294ae3376d89f98154d757cd23e3
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Nov 1 15:55:08 2018 -0400

    Merge pull request #134 from jameswnl/tower33
    
    Tower 3.3 fix: Credential.manager_ref need to be an integer for Tower 3.3
    
    (cherry picked from commit 73ba300544826bf50c8ca635c17a4b625349aeef)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1640533