Retrieve manager resource from backend and inject in Ansible Credentials form

[AparnaKarve]

The main idea here is to minimize using GETs in the angular form if possible, for a potentially better performance and a faster loading form.
Also, more importantly, I'm trying to implement a good practice of minimizing HTTP requests in the forms, given the fact that we are now getting so REST API-heavy.

In the Credentials form we have one area where we can remove the API GET call that retrieves the manager resource
...since all that we really need is the manager resource id which can very well be retrieved on the Rails side and then can be injected in the angular form.

I have some data here that gives us an idea of how much time we could potentially save by removing that extra GET call.
Note that machine speeds vary, network speeds vary, so this is really a rough ballpark.

Production mode (113 ms) -

Development mode (511 ms) -

Now 113 ms in Production mode may not sound too impressive, but I think that every little bit counts and matters in the long run, and it's a step in the right direction since it's also about the good practice of minimizing HTTP requests.
(Currently I do not have the data of how much time Rails takes to retrieve the manager resource but I'm inclined to think that it would be a less expensive operation compared to an API call.)

Hopefully, this practice of minimizing HTTP requests will be implemented across all our present/future REST API forms in simple creative ways (like in this example) either by leveraging Rails itself (applicable to Classic-UI only) or by refactoring the API.

[AparnaKarve]

@mzazrivec @himdel Please review/test and merge if you're convinced.
And comment if you're not, with a reason of course. :)

[miq-bot]

Checked commit AparnaKarve@535ff23 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 👍

[himdel]

Soo... I don't know :).

On the one hand, this definitely sounds good for performance, the code is clean and clear.. 👍

On the other, this changes code that went through the API to go via the ui-classic code instead, which is not really the direction we're trying to go.

Also, it sounds like that 511ms is a bit misleading, since 224ms out of that is spent waiting for authentications anyway.

Furthermore, the data is not really needed until you try to save, so I'd argue the timing is simply irrelevant, except for automated tests.

@mzazrivec WDYT?

---

Also, @jntullo, IIRC having to send manager_resource for API.post/put requests when dealing with embedded ansible was just a workaround for an API bug and should be fixed soon, am I right?

[AparnaKarve]

On the other, this changes code that went through the API to go via the ui-classic code instead, which is not really the direction we're trying to go.

If you recall the discussion in #641 (comment), I've been trying to get the manager resource info sent to us via an API. Not sure if that will be implemented, so what I've here is the next best choice.

Also, it sounds like that 511ms is a bit misleading, since 224ms out of that is spent waiting for authentications anyway.

Whichever way you slice it, this approach does have a better performance

Furthermore, the data is not really needed until you try to save, so I'd argue the timing is simply irrelevant, except for automated tests.

True, probably not in this case. But in general this is a good practice to use fewer HTTP requests.

[jntullo]

hmm @himdel, I am not sure what issue you are referring to. AFAIK, you will need to specify the manager_resource on the /api/authentications post/put.

however

for creating authentications on the configuration_script_payloads subcollection, (ie POST /api/configuration_script_payloads/:id/authentications) you do not need to specify a manager_resource, as it gets it from the configuration_script_payload resource.

I did notice the specs here still include the manager_resource, but that is not needed (I will create a PR to remove that because it is misleading)

[mzazrivec]

I don't like this change.

This change would move us from using rest api back to rails. Saving one http request doesn't
really cut it here. Besides, if it's the performance you're worried about, then you should have
invested the effort into finding out why the api call would take this long. Or if you want to save
one http request, then the effort should have been invested into remodeling the api so that
for embedded ansible, we would not need to make that request at all.

[himdel]

Agreed, also, it looks like we can just remove the manager_resource request altogether, since, as Jillian said, it's no longer needed by the API. (not using it for authentications, right?)

(The same should be true shortly for Repositories (configuration_script_sources)..but not quite yet :).)

[AparnaKarve]

This change would move us from using rest api back to rails

True, but I see it as a better option than the current solution.
We are injecting credential ID anyway, and for some reason that is completely acceptable, then why not also inject Manager Resource ID?

Besides, if it's the performance you're worried about,

No, tbh, performance wasn't the only thing on my mind, there was something else too...

I remember cringing, when I first saw this --

API.get('/api/providers?collection_class=ManageIQ::Providers::EmbeddedAutomationManager')
 .then(setManagerResource)
 .catch(miqService.handleFailure);

That's what I was mainly trying to target and address.

Saving one http request doesn't really cut it here

Could not agree more, and that was not my primary goal either.

I was hoping that you would see the larger picture that I was trying to portray.
In general the practice of minimizing HTTP requests in our forms is a good practice, and that's the key take-away here. Please don't be fixated on the performance aspect of that one little api call.

Anyway, this was a great discussion and I sure do appreciate everyone's comments here. So thank you for that.
The PR was a proposal anyway. If majority of the people are against it, we don't merge it, otherwise we do -- it's as simple as that.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[AparnaKarve]

The branch for the PR got deleted inadvertently while trying to do some remote github cleanup -
looks like that automatically closed this PR without my knowledge.
Anyway...
While the changes are still very valid and can impact the performance in a good way, I won't be pursuing this case atm owing to other priorities and difference of opinions from other participants. Thanks.