Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic Object wrapper method for API calls #16077

Closed
wants to merge 2 commits into from
Closed

Generic Object wrapper method for API calls #16077

wants to merge 2 commits into from

Conversation

jntullo
Copy link

@jntullo jntullo commented Sep 29, 2017

This PR creates a wrapper around the generic object method that is to be called via the API. For the methods to be called successfully, ae_user_identity must be set first. This new method, call_queued_method, will be queued by the API to set the ae_user_identity and then call the requested action with specified parameters in order to properly execute the associated automate method.

It may help to see a visual, so here it is in action being called from the API (working with what will soon be an API PR + ManageIQ/manageiq-automation_engine#74, + the always handy simulate_queue_worker).

Without parameters:
method_without_params

With parameters:
method_with_params

Naming is hard, so I am open to suggestions.

cc: @lfu @mkanoor @abellotti
@miq-bot add_label enhancement, automate
@miq-bot assign @gmcculloug

@bzwei
Copy link
Contributor

bzwei commented Sep 29, 2017

@jntullo I am wondering whether method call_queued_method is a must. You may rewrite https://github.com/ManageIQ/manageiq-api/pull/93/files#diff-57ac4c07fba9a7071def6b2f73bd1134R39 with two steps, first set the ae_user, then directly call the action, aka, property_method, just like what you did in this PR.

GenericObject is a special class. We want to define as few methods as possible while leave all the method names available to customers.

@jntullo
Copy link
Author

jntullo commented Oct 3, 2017

@bzwei unfortunately it doesn't work, since we need to queue the method. When trying your suggestion by setting the ae_user_identity prior to calling the method (via the queue), RuntimeError: A user is required to send [test_method] to automate. is raised. I am assuming this is due to the fact that the user isn't persisted.

@mkanoor
Copy link
Contributor

mkanoor commented Oct 3, 2017

@jntullo Please wait on this PR #16089 since we are changing the Queue method dispatch so that it sets the User.current_user and you dont have to.

I think when you create the Queue entry you can set the userid/groupid/tenantid and the queue worker will set up the correct context.

@jntullo
Copy link
Author

jntullo commented Oct 3, 2017

@mkanoor great, thank you!

@mkanoor
Copy link
Contributor

mkanoor commented Oct 3, 2017

@jntullo

You might want to create 2 PR's 1 in manageiq for the generic object and 1 for the manageiq-api

https://github.com/jntullo/manageiq/blob/d14d1590e86b0cffb54966fd78618d2a71daeb4e/app/models/generic_object.rb#L177

@user ||= User.current_user
@group ||= User.current_user.current_group
@tenant ||= User.current_user.current_tenant

All these variables would be set the Queue worker that is going to dispatch the work.

When you call it from the API side you have to set the user_id, group_id, tenant_id via the queue_options in MiqTask.create_generic_action_with_callback.

https://github.com/ManageIQ/manageiq-api/blob/eb172af56203e8f1747d98a17217d1d91ce715a9/app/controllers/api/base_controller/action.rb#L27

:user_id => User.current_user.id,
:group_id =>User.current_user.current_group.id,
:tenant_id => User.current_user.current_tenant.id

@mkanoor
Copy link
Contributor

mkanoor commented Oct 3, 2017

@jntullo
Since the generic object ends up running a method with Automate you would have to set the Automate role when you call the generic action with callback

https://github.com/ManageIQ/manageiq-api/blob/eb172af56203e8f1747d98a17217d1d91ce715a9/app/controllers/api/base_controller/action.rb#L27
options[:role] = 'automate'

@mkanoor
Copy link
Contributor

mkanoor commented Oct 3, 2017

@jntullo
Since user_id/group_id/tenant_id is optional we might want to pass this only for generic object methods via the options hash into
https://github.com/ManageIQ/manageiq-api/blob/eb172af56203e8f1747d98a17217d1d91ce715a9/app/controllers/api/base_controller/action.rb#L16

This would allow us to slowly add this functionality as and when we need user context. If we introduce it directly inside of queue_object_action all the methods would be running within a specific user context and the results might not be what you are expecting.

@miq-bot
Copy link
Member

miq-bot commented Oct 4, 2017

Checked commits jntullo/manageiq@5d1a3e8~...23b428c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

spec/models/generic_object_spec.rb

@jntullo
Copy link
Author

jntullo commented Oct 4, 2017

closing in favor of #16120

@jntullo jntullo closed this Oct 4, 2017
@jntullo jntullo deleted the enhancement/generic_object_wrapper branch November 28, 2017 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants