Add basic CentralAdmin support to the API #472

bdunne · 2018-09-19T22:23:17Z

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

bdunne · 2018-09-19T22:23:55Z

I think I'm going to refactor the tests to reduce duplication, but I wanted to open this to see if anyone had any major concerns.

bdunne · 2018-09-19T22:24:48Z

@gtanzillo @jrafanie This wound up being a much cleaner implementation than the others we discussed, and it's easily extendable in the future.

carbonin · 2018-09-20T13:45:48Z

app/controllers/api/vms_controller.rb

@@ -152,6 +159,7 @@ def scan_resource(type, id = nil, _data = nil)
        result
      end
    end
+    central_admin :scan_resource, :scan


There are a few of these in here that I don't think we support from the main app.

Doing a quick look for api_relay in the core source I can only find shutdown_guest, reboot_guest, reset, start, stop, and suspend. We should keep the actions we support consistent between the UI and API.

Yeah, they were simple, so I added them.

Also, I'd like to add all future central admin features through the api and maybe even deprecate the central admin support in the models.

I wouldn't introduce new behavior just to make it complete. We can add the new ones later when they're needed.

I still think we should leave out the extra methods here until they are also supported through the classic UI. If this PR is meant to solve a particular BZ we should limit the scope that issue rather than also adding new features.

Since enabling them all is so easy, I figured I would just do it proactively rather than waiting for another high severity customer case saying Action X doesn't work.

Right, but as I understand it this was because of missing operations in the API vs the classic UI. If you really do want to add the new ones, then please create an additional PR to add the same operations in core for consistency.

I can see the value in separating the bugfix from the enhancement part of this PR. 👍

Okay, I removed the things we don't central admin in the UI. I'll open a followup PR to add those.

jrafanie · 2018-09-20T21:34:33Z

app/controllers/api/mixins/central_admin.rb

+      end
+    end
+  end
+end


jrafanie · 2018-09-20T21:45:36Z

Wow, and this works? Looks good. My only concern is what @carbonin brought up above

bdunne · 2018-09-21T18:17:39Z

Wow, and this works?

For better or worse, it does the same thing that the central admin through the classic-ui does. So, the actions get passed down properly, but the response returned to the user is garbage.

Valid action:

$ api -l https://global_region post /api/vms/2
{
  "action" : "stop"
}

{
  "error": {
    "kind": "internal_server_error",
    "message": "undefined method `keys' for nil:NilClass",
    "klass": "NoMethodError"
  }
}

$ api -l https://remote_region post /api/vms/2
{
  "action" : "stop"
}

{
  "success": true,
  "message": "VM id:2 name:'Apache_PHP_rh_summit_002' stopping",
  "task_id": "4",
  "task_href": "https://remote_region/api/tasks/4",
  "href": "https://remote_region/api/vms/2"
}

Invalid action:

$ api -l https://global_region post /api/vms/3
{
  "action" : "start"
}

{
  "error": {
    "kind": "internal_server_error",
    "message": "The VM is retired",
    "klass": "InterRegionApiMethodRelay::InterRegionApiMethodRelayError"
  }
}

$ api -l https://remote_region post /api/vms/3
{
  "action" : "start"
}

{
  "success": false,
  "message": "The VM is retired",
  "href": "https://remote_region/api/vms/3"
}

I'm investigating what needs to be done to make the response useful.

jrafanie · 2018-09-21T18:37:49Z

I'm investigating what needs to be done to make the response useful.

Interesting edge cases you're testing. It's a great idea to try vms in various states.

bdunne · 2018-10-02T11:25:24Z

Okay, @carbonin @jrafanie @gtanzillo @abellotti This is ready to go. I got the success and failure responses working properly and all of the hrefs pointing to the correct server.

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

- pause - reboot_guest - refresh - reset - scan - shelve - shelve_offload - shutdown_guest - start - stop - suspend Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1628658

- pause - reboot_guest - reset - shelve - start - stop - suspend - terminate Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1628658

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

miq-bot · 2018-10-02T16:24:28Z

Checked commits bdunne/manageiq-api@5d11ea8~...504eda4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 🏆

jrafanie

LGTM, I can't merge here.

@bdunne and I want to create a new label and call it bughancement with the color of the existing two labels added together.

bdunne · 2018-10-02T19:55:19Z

@bdunne and I want to create a new label and call it bughancement with the color of the existing two labels added together.

🤣

gtanzillo

Looks awesome! 👍

simaishi · 2018-10-05T17:28:10Z

@bdunne hammer/yes and gaprindashvili/yes ?

bdunne · 2018-10-05T17:35:44Z

@simaishi I think so. Also related are: ManageIQ/manageiq#17987 and ManageIQ/manageiq#18036

Add basic CentralAdmin support to the API (cherry picked from commit 4dbf6b0) https://bugzilla.redhat.com/show_bug.cgi?id=1628658

simaishi · 2018-10-08T12:32:28Z

Hammer backport details:

$ git log -1
commit 60cb7c906f7ec9724e5ac56f76e2558bfae1fd0b
Author: Gregg Tanzillo <[email protected]>
Date:   Wed Oct 3 08:56:43 2018 -0400

    Merge pull request #472 from bdunne/central_admin
    
    Add basic CentralAdmin support to the API
    
    (cherry picked from commit 4dbf6b0bbb3cc10be69d6259c069e385ee7bc10c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1628658

Add basic CentralAdmin support to the API (cherry picked from commit 4dbf6b0) https://bugzilla.redhat.com/show_bug.cgi?id=1635764

simaishi · 2018-10-10T19:53:58Z

Gaprindashvili backport details:

$ git log -1
commit 96b8096ee80d56ac45faee5fe73328443a4ff085
Author: Gregg Tanzillo <[email protected]>
Date:   Wed Oct 3 08:56:43 2018 -0400

    Merge pull request #472 from bdunne/central_admin
    
    Add basic CentralAdmin support to the API
    
    (cherry picked from commit 4dbf6b0bbb3cc10be69d6259c069e385ee7bc10c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1635764

bdunne added bug enhancement labels Sep 19, 2018

bdunne requested review from gtanzillo, abellotti and carbonin September 19, 2018 22:23

bdunne requested a review from jrafanie September 19, 2018 22:24

carbonin reviewed Sep 20, 2018

View reviewed changes

jrafanie reviewed Sep 20, 2018

View reviewed changes

app/controllers/api/mixins/central_admin.rb

end

end

end

end

Copy link

Member

jrafanie Sep 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enddddddd

bdunne reacted with laugh emoji

bdunne force-pushed the central_admin branch 2 times, most recently from 621ced9 to 9e11519 Compare October 2, 2018 11:12

bdunne added 6 commits October 2, 2018 12:09

Add CentralAdmin support to the API

5d11ea8

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

Add CentralAdmin support to /api/vms for:

23b8a38

- pause - reboot_guest - refresh - reset - scan - shelve - shelve_offload - shutdown_guest - start - stop - suspend Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1628658

Add CentralAdmin support to /api/instances for:

81bb2b7

- pause - reboot_guest - reset - shelve - start - stop - suspend - terminate Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1628658

Refactor into vms and instances shared_examples

9922180

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

Move central admin tests into existing spec files

f685194

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

Existing tests should be using the current region

504eda4

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

bdunne force-pushed the central_admin branch from 9e11519 to 504eda4 Compare October 2, 2018 16:21

carbonin approved these changes Oct 2, 2018

View reviewed changes

jrafanie approved these changes Oct 2, 2018

View reviewed changes

gtanzillo approved these changes Oct 2, 2018

View reviewed changes

gtanzillo self-assigned this Oct 2, 2018

gtanzillo added this to the Sprint 96 Ending Oct 8, 2018 milestone Oct 3, 2018

gtanzillo merged commit 4dbf6b0 into ManageIQ:master Oct 3, 2018

bdunne deleted the central_admin branch October 3, 2018 13:49

bdunne added gaprindashvili/yes labels Oct 5, 2018

simaishi pushed a commit that referenced this pull request Oct 8, 2018

Merge pull request #472 from bdunne/central_admin

60cb7c9

Add basic CentralAdmin support to the API (cherry picked from commit 4dbf6b0) https://bugzilla.redhat.com/show_bug.cgi?id=1628658

simaishi added hammer/backported and removed hammer/yes labels Oct 8, 2018

simaishi mentioned this pull request Oct 8, 2018

Prevent queueing things for a zone that doesn't exist in the region ManageIQ/manageiq#17987

Merged

simaishi added the blocker label Oct 8, 2018

simaishi mentioned this pull request Oct 9, 2018

[GAPRINDASHVILI] Prevent queueing things for a zone that doesn't exist in the region ManageIQ/manageiq#18068

Merged

simaishi pushed a commit that referenced this pull request Oct 10, 2018

Merge pull request #472 from bdunne/central_admin

96b8096

Add basic CentralAdmin support to the API (cherry picked from commit 4dbf6b0) https://bugzilla.redhat.com/show_bug.cgi?id=1635764

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Oct 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic CentralAdmin support to the API #472

Add basic CentralAdmin support to the API #472

bdunne commented Sep 19, 2018

bdunne commented Sep 19, 2018

bdunne commented Sep 19, 2018

carbonin Sep 20, 2018

bdunne Sep 20, 2018

bdunne Sep 20, 2018

jrafanie Sep 20, 2018

carbonin Oct 2, 2018

bdunne Oct 2, 2018

carbonin Oct 2, 2018

jrafanie Oct 2, 2018

bdunne Oct 2, 2018

jrafanie Sep 20, 2018

jrafanie commented Sep 20, 2018

bdunne commented Sep 21, 2018

jrafanie commented Sep 21, 2018

bdunne commented Oct 2, 2018

miq-bot commented Oct 2, 2018

jrafanie left a comment

bdunne commented Oct 2, 2018

gtanzillo left a comment

simaishi commented Oct 5, 2018

bdunne commented Oct 5, 2018

simaishi commented Oct 8, 2018

simaishi commented Oct 10, 2018

Add basic CentralAdmin support to the API #472

Add basic CentralAdmin support to the API #472

Conversation

bdunne commented Sep 19, 2018

bdunne commented Sep 19, 2018

bdunne commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Sep 20, 2018

bdunne commented Sep 21, 2018

jrafanie commented Sep 21, 2018

bdunne commented Oct 2, 2018

miq-bot commented Oct 2, 2018

jrafanie left a comment

Choose a reason for hiding this comment

bdunne commented Oct 2, 2018

gtanzillo left a comment

Choose a reason for hiding this comment

simaishi commented Oct 5, 2018

bdunne commented Oct 5, 2018

simaishi commented Oct 8, 2018

simaishi commented Oct 10, 2018