Add explicit Endpoint#to_s method to return url

[djberg96]

Just a handy method to return the url of the endpoint when Endpoint#to_s is called.

This will be useful for endpoint support for Azure and Amazon, where internally I can call .to_s and it won't matter if it's a URI object or and Endpoint object, it will return what I expect - the url. Currently it just returns the object ID, which is not useful, and forces kind_of checks and whatnot.

[juliancheal]

LGTM 👍

[Fryguy]

I don't think all Endpoints have a URL, such as RHV (which does have a URL, but it is constructed). This feels weird for those cases. Additionally, for something like vSphere, I'd expect it to returm nil as opposed to ''.

[Fryguy]

This will be useful for endpoint support for Azure and Amazon, where internally I can call .to_s and it won't matter if it's a URI object or and Endpoint object, it will return what I expect - the url. Currently it just returns the object ID, which is not useful, and forces kind_of checks and whatnot.

Do you have a link to this code?...very curious how this is being used.

[djberg96]

Right, not all endpoints will have a URL so it will just be a blank string. Having to_s return anything but a string would violate POLS in my opinion. Internally we can check for .blank?. I wouldn't think this would affect any other provider anyway.

ManageIQ/manageiq-providers-azure#274

Internally this could be either a URI object (if received from the UI), or an Endpoint (when re-checking authentication status), so I'm having to make an extra call. With this in place, I can call to_s regardless of type and it will Just Work.

[kbrock]

I think the tests sell it for me.
They respond the way I was expecting

[miq-bot]

Checked commits https://github.com/djberg96/manageiq/compare/9239e7a9c1213d3df2a01ca51d50fdf8b452d499~...f4f4e98b746c1b6146cf0489b6327ba8652dfa40 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

[jrafanie]

The tests work exactly how I'd expect...

@Fryguy re:

I don't think all Endpoints have a URL, such as RHV (which does have a URL, but it is constructed). This feels weird for those cases. Additionally, for something like vSphere, I'd expect it to returm nil as opposed to ''.

I think the nil URL is handled correctly. If you ask for url, it should be nil. If you try to call .to_s, it should be an empty string. That's exactly what I'd expect. Can you clarify if this is not your expectation? I'd good with this change although this would be the one time I would have squashed the commits.

[bdunne]

I think what @Fryguy meant was that Endpoint#to_s for AnsibleTower will give you a URL in a string, but for Vmware, you'll get an empty string because we don't use the url column and store all of the URI components in their own columns instead. This difference in behavior is confusing if you don't know that.

[djberg96]

@bdunne That's an issue for the VMWare provider then. Either VMWare should start storing the URL, or override the url method in the model so that it joins the components to create a URL. Or just not use this method.

[jrafanie]

@bdunne That's an issue for the VMWare provider then. Either VMWare should start storing the URL, or override the url method in the model so that it joins the components to create a URL. Or just not use this method.

I agree. My naive thinking is most endpoints will have a URI/URL and if you don't, you need to implement url or even to_s with whatever makes sense.

[bdunne]

My naive thinking is most endpoints will have a URI/URL

In reality it's the minority of Providers and ExtManagementsSystems that have a url, most store the components instead

[jrafanie]

In reality it's the minority of Providers and ExtManagementsSystems that have a url, most store the components instead

Maybe the ems/providers should implement their own to_s then.

[djberg96]

@jrafanie This should be the base IMO, and the providers can override it if necessary.

[kbrock]

Um. this is a to_s method that works the way I expect it to work.

I'm probably missing something but...
Do we really care about this enough to hold up merging it for over 6 months?

[jrafanie]

@djberg96 I can see the use case for treating an endpoint as a URL like we do a pathname for a path. I didn't at first, but reviewing ALL the methods and associations on the Endpoint class, they all have to do with communicating with the location found in the URL. In other words, the URL best describes the endpoint object. Therefore, I'm good with making this general solution and any subclasses that don't want this behavior can either provide their own to_s or implement url as they see fit.

I think the concerns from @Fryguy and @bdunne can be addressed in subclasses override methods if they want to do something else. Just looking at endpoint.rb, it's clear the URL is the endpoint and everything else is just details for that URL.

I'd like @agrare to review this but I'm 👍

[kbrock]

Everyone deserves a uri. postgres, redis, memcached all have them. vmware can join the party by overriding this class if they want.

[djberg96]

Yay, thanks everyone!

[Fryguy]

I was actually 👎 on this change and I believe @bdunne was as well, so I'm surprised this was merged.

Everyone deserves a uri.

I have no idea what this means.

Um. this is a to_s method that works the way I expect it to work.

This is a .to_s on a Rails model which means this one model works completely different than any other. It also doesn't solve my main issue which is that .uri is not the primary component of an endpoint for many providers.

[agrare]

I don't think this is a VMware thing, no InfraManagers use the url field of an endpoint (because the client gems don't ask for one, they just want the hostname/port/ssl info). Just double checked with RHV as well.

I don't know why we would set a URL if it isn't going to be used by anyone? I don't have a problem with setting it but not sure what problem that would be fixing.

[Fryguy]

URL won't be set in infra managers, but I guess it could. I just really don't like the idea of an entire model essentially being represented by one column, when in reality the model is more than that.

[kbrock]

I'm sorry for the quick trigger.

The ui will not use to_s. This just seems like an easy way for developers to view an endpoint and get their job done.
The to_s will only be used in Azure's raw_connect. No one else needs to use it.

I'll put together an alternative to_s implementation. You can either/or/and:

a) roll this back
b) review the other PR