Remove extra authentication created when editing ems_container

[yaacov]

Description
Remove the extra "default" authentication created when editing a container provider.

Screenshots
An extra endpoint named "default" is created

With this fix only "bearer" and "hawkular" are created

Bugzilla
https://bugzilla.redhat.com/show_bug.cgi?id=1361012

Issue
#9946

[yaacov]

@miq-bot add_label providers/containers, ui, bug

@simon3z @AparnaKarve @zeari please review

[miq-bot]

Checked commit yaacov@02cd778 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍪

[simon3z]

@miq-bot assign yaacov

[Fryguy]

This feels strange to be a UI thing...it also lends itself to breaking UI provider agnosticism. I would say that alternately we should not be creating a default auth at all if it's not used, but then again, the common practice is that default is the main connection...in this case what has been called "bearer" I believe. cc @blomquisg @durandom Thoughts?

[durandom]

it also lends itself to breaking UI provider agnosticism

ems_common is the opposite of provider agnostic 😁

@juliancheal is tackling this. And until we have a clean way to support stuff like this on the provider side, I am ok with adding more provider specifics in here, because its just not reasonably possible in another way 😒

[yaacov]

@Fryguy

we should not be creating a default auth at all if it's not used

the default authentication is created if params[:default_userid] exists:
https://github.com/ManageIQ/manageiq/pull/9947/files#diff-f402e411cd6dc158c724b14d72ead612L399
I can add an && !ems.kind_of?(ManageIQ::Providers::ContainerManager) in line 399, but it will not make this more pretty 😢

[yaacov]

@Fryguy I like @AparnaKarve 's idea in #9947 (comment), WDYT ?

[blomquisg]

IMO the build_credentials method needs a lot of work, of which Containers is going to be just one part.

Looking up higher in the method reveals something of a Jenga game of stacking logic.

Let's stick with how @yaacov has this for now. And, put this on the list for refactoring (cc @juliancheal, add this to your list :) )

[Fryguy]

@simon3z I switched the assignment back to you as it doesn't make sense to assign a PR to the author :)

[yaacov]

@Fryguy

I would say that alternately we should not be creating a default auth at all if it's not used

In #9235 It was decided that in the UI the default authentication will be called default, I do not know If we want to change this back to bearer now @AparnaKarve ?

[AparnaKarve]

@yaacov If I remember correct, in the UI (i.e. in the EMS Container form) we always displayed the word default for the authentication of type bearer.
What was changed in #9235 was, all the variables in the JS and the haml that were being called bearer (for e.g. bearer_password) were changed to default (default_password), which greatly simplified the JS code.

EDIT: I think by UI, you meant the summary screen.
So that means there is a slight discrepancy in the UI -- the EMS form calls it default and the Summary screen calls it bearer.
If we decide to go back to bearer (as in bearer_userid, bearer_password), then I suppose the UI (JS) code would have to make many many adjustments around that. IMO, It would be much more easier to tackle it in ems_common_angular.rb (which we are already doing)

How about we change the code in line 399 to something like this, that way we do not have to delete creds[:default]

if params[:default_userid]
 default_password = params[:default_password] ? params[:default_password] : ems.authentication_password
 auth_type = ems.supports_authentication?(:bearer) ? :bearer : :default
 creds[auth_type] = {:userid => params[:default_userid], :password => default_password, :save => (mode != :validate)}
end

[simon3z]

@simon3z I switched the assignment back to you as it doesn't make sense to assign a PR to the author :)

@Fryguy the assignee field specifies on whose plate a specific issues/pr is standing. At the moment this is on Yaacov's side as he's still addressing comments.

Also the agreement with @chessbyte is that once the PR is ready it should be assigned to one of you guys to be merged.
So the assignee helps me understand if it's on our plate (and on which engineer) or if it's on you guys.

The assignees are also collected by our stats scripts, so unless this is breaking the process on your side I'll keep the previous assignee until the PR is ready.

@miq-bot assign yaacov

[simon3z]

Greg, we're ready here. Can you check if this could be accepted? Thanks.

@miq-bot assign blomquisg

[chessbyte]

@simon3z @blomquisg Darga?

[simon3z]

@chessbyte I'll have to discuss this with @yaacov . Maybe it will become Darga material, but I wouldn't rush this in for this release.

@yaacov ping me tomorrow about this one. Thanks.

[simon3z]

@chessbyte I discussed this with @yaacov and we need it in darga.

@miq-bot add_label darga/yes

[chessbyte]

Darga backport details:

commit 9a29b67150e24de9492076dae63641264f66e8aa
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Mon Aug 8 10:59:14 2016 -0400

    Merge pull request #9947 from yaacov/remove-extra-default-authentication-for-ems-containers

    Remove extra authentication created when editing ems_container
    (cherry picked from commit c036c75d58051dfeb262c6526a518d16b27df719)