Request cancel #421

bdunne · 2018-07-13T16:24:34Z

Based on changes in: ManageIQ/manageiq#17687
https://bugzilla.redhat.com/show_bug.cgi?id=1564257
cc @abellotti @gtanzillo @gmcculloug @bzwei

bzwei · 2018-07-13T17:30:20Z

@bdunne v2v RFE needs task level cancellation. Do we expect another PR?

gmcculloug · 2018-07-16T12:20:31Z

@bdunne Since the back-end is really only supporting this feature for v2v can we reduce the models this is exposed on? At the moment we return "Unsupported" for all other request/task types so I would prefer limiting the action as much as possible.

bdunne · 2018-07-16T16:14:54Z

@gmcculloug Many thoughts..

I was surprised to find out that we have 4 requests endpoints. Do you know why?
I'd like keep these 4 endpoints as similar as possible as long as it makes sense (especially in this case where 2/4 would have to implement it in my opinion)
I think an "Unsupported" response is better than "Bad Request Error"
It may only be used by v2v at the moment, but I don't think the API should care about implementing things for specific use cases, it should be done in a generic and consistent way.

gmcculloug · 2018-07-17T13:26:42Z

I was not involved in building out the API so do not have any insight into why there are 4 endpoints.

@gtanzillo @abellotti Are likely to know the answer.

JPrause · 2018-07-17T13:38:42Z

@miq-bot add_label blocker

JPrause · 2018-07-17T13:39:13Z

@bdunne if this can be backported, can you add the gaprindashvili/yes label

gmcculloug · 2018-07-19T14:14:28Z

@gtanzillo @abellotti Any thoughts on #421 (comment)

gtanzillo · 2018-07-19T15:58:00Z

@gmcculloug Regarding

I was not involved in building out the API so do not have any insight into why there are 4 endpoints.

@gtanzillo @abellotti Any thoughts on #421 (comment)

I was also not involved so I'm not sure either. It seems in some places we've exposed a single endpoint for the base class and in other places we exposed separate endpoints for each subclass like was done here.

Perhaps @abellotti or @Fryguy can comment on that. It would be good to understand which convention we should be following, if there is one.

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

bdunne · 2018-07-19T19:01:38Z

Okay, I think this is ready for review. @jrafanie can you put another set of 👀 on the tests?

jrafanie · 2018-07-19T20:33:07Z

LGTM. It's a bit hard to follow at first with the indirection but the alternative is 1000 lines of duplicated tests, so

It would be good if @Fryguy can comment on the separate collections for each subclass. It seems strange. I can't imagine having separate collections for vmware vms, redhat vms, etc. On the fipside, these things are not the same and are actual tasks for completely different end user features. 🤷‍♂️

Fryguy · 2018-07-19T20:34:52Z

app/controllers/api/mixins/resource_cancel.rb

@@ -0,0 +1,14 @@
+module Api
+  module Mixins
+    module ResourceCancel


Should this be ResourceCancel or TaskCancel?

It's for ServiceOrder, MiqRequestX4 or MiqRequestTask primary collections.

Fryguy · 2018-07-19T20:37:07Z

It would be good if @Fryguy can comment on the separate collections for each subclass. It seems strange. I can't imagine having separate collections for vmware vms, redhat vms, etc.

@jrafanie Where are you seeing this?

Fryguy · 2018-07-19T20:38:54Z

spec/support/api/helpers.rb

@@ -224,6 +224,12 @@ def expect_options_results(type, data = {})
          expect(response.headers['Access-Control-Allow-Methods']).to include('OPTIONS')
        end

+        def expect_unauthorized_request


minor, but this might read better as expect_forbidden_request, since the status is :forbidden

bdunne · 2018-07-19T21:12:29Z

@jrafanie Where are you seeing this?

/api/requests
/api/automation_requests
/api/service_requests
/api/provision_requests

Talking to Alberto, it sounds like it just grew this way over time. Hopefully we don't need to create new endpoints for the other 12 subclasses of MiqRequest.

miq-bot · 2018-07-25T15:29:34Z

Checked commits bdunne/manageiq-api@7cad510~...1064cf9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
19 files checked, 9 offenses detected

app/controllers/api/mixins/resource_cancel.rb

❗ - Line 9, Col 7 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

app/controllers/api/subcollections/request_tasks.rb

❗ - Line 22, Col 7 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

app/controllers/api/subcollections/service_requests.rb

❗ - Line 33, Col 7 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

spec/requests/automation_requests_spec.rb

❗ - Line 299, Col 45 - Style/SymbolArray - Use %i or %I for an array of symbols.

spec/requests/provision_requests_spec.rb

❗ - Line 489, Col 45 - Style/SymbolArray - Use %i or %I for an array of symbols.

spec/requests/requests_spec.rb

❗ - Line 526, Col 45 - Style/SymbolArray - Use %i or %I for an array of symbols.

spec/requests/service_orders_spec.rb

❗ - Line 636, Col 45 - Style/SymbolArray - Use %i or %I for an array of symbols.

spec/requests/service_requests_spec.rb

❗ - Line 638, Col 45 - Style/SymbolArray - Use %i or %I for an array of symbols.

spec/requests/service_templates_spec.rb

❗ - Line 312, Col 45 - Style/SymbolArray - Use %i or %I for an array of symbols.

AparnaKarve · 2018-07-27T14:36:16Z

Can we get this merged today?

bthurber · 2018-07-30T16:37:35Z

@gtanzillo @gmcculloug @blomquisg we will need this merged ASAP for 5.9.4-2 build for tomorrow. Anything here left to finish/review?

bthurber · 2018-07-30T18:41:47Z

@bdunne can you respond to @Fryguy last comment? We also need to add enhancement, transformation and grapendashvili/yes labels so this get's pulled into the next build once merged.

bdunne · 2018-07-30T19:17:09Z

@bthurber I responded in: #421 (comment)

bdunne · 2018-07-30T19:17:59Z

@miq-bot add_labels gaprindashvili/yes enhancement transformation

miq-bot · 2018-07-30T19:18:20Z

@bdunne Cannot apply the following label because they are not recognized: gaprindashvili/yes enhancement transformation

bdunne · 2018-07-30T19:35:06Z

@miq-bot add_labels gaprindashvili/yes
@miq-bot add_labels enhancement
@miq-bot add_labels transformation

Request cancel (cherry picked from commit 62de390) https://bugzilla.redhat.com/show_bug.cgi?id=1610533

simaishi · 2018-07-31T21:25:23Z

Gaprindashvili backport details:

$ git log -1
commit b7047744b21596fafe4cbfdc1064cad1fd8ba76c
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Mon Jul 30 18:02:12 2018 -0400

    Merge pull request #421 from bdunne/request_cancel
    
    Request cancel
    (cherry picked from commit 62de390490c8bc333f27feedfc58455ced1e7483)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1610533

bdunne force-pushed the request_cancel branch from 0d030d8 to 7b277e5 Compare July 13, 2018 16:43

bzwei mentioned this pull request Jul 16, 2018

User can cancel a migration in progress ManageIQ/manageiq-v2v#370

Closed

miq-bot added the blocker label Jul 17, 2018

gtanzillo requested a review from abellotti July 17, 2018 21:08

bdunne force-pushed the request_cancel branch 2 times, most recently from d537d04 to c3902ee Compare July 18, 2018 19:03

bdunne changed the title ~~Request cancel~~ [WIP] Request cancel Jul 19, 2018

miq-bot added the wip label Jul 19, 2018

bdunne force-pushed the request_cancel branch 2 times, most recently from 8bb22e4 to 5317105 Compare July 19, 2018 15:17

bdunne added 3 commits July 19, 2018 14:48

Allow href, message and success as string or symbol

7cad510

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

Add a helper to simplify testing unauthorized requests

581d8d4

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

Add request#cancel support to all 4 requests controllers

d7d0087

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

bdunne force-pushed the request_cancel branch from 5317105 to 909fbe2 Compare July 19, 2018 18:49

bdunne changed the title ~~[WIP] Request cancel~~ Request cancel Jul 19, 2018

bdunne added 4 commits July 19, 2018 14:58

Add subcollection RequestTask#cancel

63080a8

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

Add primary collection RequestTasks#cancel

b7dc2d9

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

Allow instance and bulk cancel of service_template service_requests

e83ed44

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

Modify the factories to set user rather than try everywhere

80b686e

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

bdunne force-pushed the request_cancel branch from 909fbe2 to 80b686e Compare July 19, 2018 18:58

miq-bot removed the wip label Jul 19, 2018

Fryguy reviewed Jul 19, 2018

View reviewed changes

abellotti approved these changes Jul 25, 2018

View reviewed changes

Rename unauthorized request helper to forbidden

1064cf9

AparnaKarve mentioned this pull request Jul 26, 2018

[#370] Cancel a migration's in-progress tasks ManageIQ/manageiq-v2v#517

Merged

miq-bot added gaprindashvili/yes enhancement v2v labels Jul 30, 2018

gtanzillo self-assigned this Jul 30, 2018

gtanzillo approved these changes Jul 30, 2018

View reviewed changes

gtanzillo added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 30, 2018

gtanzillo merged commit 62de390 into ManageIQ:master Jul 30, 2018

bdunne deleted the request_cancel branch July 30, 2018 22:02

AparnaKarve mentioned this pull request Jul 31, 2018

[GAPRINDASHVILI] Cancel a migration's in-progress tasks ManageIQ/manageiq-v2v#525

Merged

simaishi pushed a commit that referenced this pull request Jul 31, 2018

Merge pull request #421 from bdunne/request_cancel

b704774

Request cancel (cherry picked from commit 62de390) https://bugzilla.redhat.com/show_bug.cgi?id=1610533

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request cancel #421

Request cancel #421

bdunne commented Jul 13, 2018

bzwei commented Jul 13, 2018

gmcculloug commented Jul 16, 2018

bdunne commented Jul 16, 2018

gmcculloug commented Jul 17, 2018

JPrause commented Jul 17, 2018

JPrause commented Jul 17, 2018

gmcculloug commented Jul 19, 2018

gtanzillo commented Jul 19, 2018

bdunne commented Jul 19, 2018

jrafanie commented Jul 19, 2018

Fryguy Jul 19, 2018 •

edited

Loading

bdunne Jul 19, 2018

Fryguy commented Jul 19, 2018 •

edited

Loading

Fryguy Jul 19, 2018

bdunne commented Jul 19, 2018

miq-bot commented Jul 25, 2018

AparnaKarve commented Jul 27, 2018

bthurber commented Jul 30, 2018

bthurber commented Jul 30, 2018

bdunne commented Jul 30, 2018

bdunne commented Jul 30, 2018

miq-bot commented Jul 30, 2018

bdunne commented Jul 30, 2018

simaishi commented Jul 31, 2018

Request cancel #421

Request cancel #421

Conversation

bdunne commented Jul 13, 2018

bzwei commented Jul 13, 2018

gmcculloug commented Jul 16, 2018

bdunne commented Jul 16, 2018

gmcculloug commented Jul 17, 2018

JPrause commented Jul 17, 2018

JPrause commented Jul 17, 2018

gmcculloug commented Jul 19, 2018

gtanzillo commented Jul 19, 2018

bdunne commented Jul 19, 2018

jrafanie commented Jul 19, 2018

Fryguy Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

bdunne Jul 19, 2018

Choose a reason for hiding this comment

Fryguy commented Jul 19, 2018 • edited Loading

Fryguy Jul 19, 2018

Choose a reason for hiding this comment

bdunne commented Jul 19, 2018

miq-bot commented Jul 25, 2018

AparnaKarve commented Jul 27, 2018

bthurber commented Jul 30, 2018

bthurber commented Jul 30, 2018

bdunne commented Jul 30, 2018

bdunne commented Jul 30, 2018

miq-bot commented Jul 30, 2018

bdunne commented Jul 30, 2018

simaishi commented Jul 31, 2018

Fryguy Jul 19, 2018 •

edited

Loading

Fryguy commented Jul 19, 2018 •

edited

Loading