Fixes #36015 - API endpoints to add/remove single roles #597

ofedoren · 2023-02-16T17:52:37Z

No description provided.

nofaralfasi · 2023-02-21T12:37:53Z

app/controllers/foreman_ansible/api/v2/hostgroups_controller_extensions.rb

+          api :DELETE, '/hostgroups/:id/remove_ansible_roles',
+              N_('Remove directly assigned Ansible roles from a hostgroup')
+          param :id, :identifier, :required => true
+          param :ansible_role_ids, Array,


How do you pass an array of ansible_role_ids? I wasn't able to do that (but it works with just one ansible_role_id)

You can simply use JSON object with array: { "ansible_role_ids": [1,2,3] }. Accuracy depends on what you use. Since curl needs to be pretty verbose, I use Postman Chrome extension which makes API testing quite easy and straightforwad.

nofaralfasi · 2023-02-22T10:44:14Z

test/functional/api/v2/hosts_controller_test.rb

+        host = FactoryBot.create(:host,
+                                 :managed => false,
+                                 :ansible_role_ids => [@ansible_role3.id, @ansible_role2.id, @ansible_role1.id])
+        post :remove_ansible_roles,


The only thing I couldn't figure out yet is how the tests are passing even though it is a post request (same for hostgroup). Should it be a delete request?

That's a good question... :/

I think @adamruzicka describes it pretty well here

ekohl

I'm not commenting inline, since /add_X and remove_x are used in both the hostgroup and the hosts controllers. It's not a very REST thing include the action in the endpoint: the HTTP verb describes what you want.

In REST you have collections and resources. A collection is /hosts and a resource is /hosts/:id. Then the HTTP verb describes the action.

More practical, that means you have a collection /hosts/:id/ansible/roles. You can GET them to get a listing, POST to it to create a new link, which you also direct you to the individual resources.

The individual resources would likely be on /hosts/:host_id/ansible/roles/:role_id, which you can GETto view andDELETEto remove. If you want to update it,PUT` is typically used.

https://restfulapi.net/resource-naming/ does mention that sometimes you do have a more procedural concept, but in this case it maps very well to collections and resources.

Now I will note you here add and delete collections, but https://restfulapi.net/http-methods/#delete really focuses on individual resources. That's what REST is all about. The PR summary describes you say add/remove single roles, so per resource is probably sufficient for that. If that's really what you want to support bulk actions, I'm not sure REST is correct. If you want it in the UI, the graphql API may be more appropriate than REST.

ekohl · 2023-03-30T13:18:02Z

app/controllers/foreman_ansible/api/v2/hostgroups_controller_extensions.rb

+                :required => true
+
+          def add_ansible_role
+            process_response @hostgroup.ansible_roles << @ansible_role


What if the role is already in there? Is ansible_roles a set or a list?

If the role is already assigned, it will return

{ "error": { "message": "Validation failed: Ansible role has already been taken" } }

with 500 error code.

A HTTP 500 sounds bad. I'd consider making it 422 if you consider it wrong or 200 if you think it's OK (and I'd use 200 Ok).

You can also consider an API endpoint PUT /hostgroups/:id/ansible_roles/:ansible_role_id which should be idempotent: it ensures it's part of the set.

I agree that 500 code is weird for such response, but Rails will treat this as a validation error anyway. I left 422 instead of suppressing the error. If you still find this wrong, I can change it to suppress the ActiveRecord::RecordInvalid and return 200.

ekohl · 2023-03-30T13:18:26Z

test/functional/api/v2/hostgroups_controller_test.rb

+               :ansible_role_id => [@ansible_role2.id]
+             },
+             :session => set_session_user
+        assert_response :success


I'd expect HTTP 201 Created if it was added, not HTTP 200 Success

The server responds with correct 201.

P.S. Apparently post and others are expecting :action instead of route from routes definition. ofedoren.experience += 1. Also, assert_response here considers both 200 and 201 as successful via mapping, that's why the test doesn't fail, but I've changed to use the specific number instead.
P.P.S. I hate test frameworks :/

ekohl · 2023-03-30T13:20:06Z

test/functional/api/v2/hostgroups_controller_test.rb

+                 :ansible_role_id => [@ansible_role2.id]
+               },
+               :session => set_session_user
+        assert_response :success


HTTP 204 (No Content) is common after a DELETE. If it's 200 Success, I wonder what's in the content.

We have the same behaviour with responses almost everywhere else since we use the same logic/methods. In this case the server will respond with 200 and the modified entity (hostgroup in this case).

ekohl · 2023-03-30T13:22:08Z

test/functional/api/v2/hostgroups_controller_test.rb

+        assert_equal assigns('hostgroup').ansible_roles, [@ansible_role1, @ansible_role2]
+      end
+
+      test 'should remove only specified roles from a hostgroup' do


Another use case: what if the role doesn't exist? Does it return 404 or 200/204 since it's not part of the role collection anymore? I'd expect the latter.

The server will respond with 200.

ofedoren · 2023-04-11T11:47:53Z

[test foreman_ansible]

Oh, right, github actions... There really should be a button/bot command to re-run the tests :/

nofaralfasi

LGTM. Everything works as expected.
@ekohl can I merge? :)

nofaralfasi · 2023-05-08T12:09:37Z

I will merge it to ensure it's included in the next released version.
If there are any additional changes needed, we can open a RM issue to address them separately.
Thanks @ofedoren!

theforeman-bot added the Not yet reviewed label Feb 16, 2023

ofedoren force-pushed the feat-36015-add-role-by-one branch 2 times, most recently from 186eda3 to 8434de4 Compare February 17, 2023 12:32

nofaralfasi reviewed Feb 21, 2023

View reviewed changes

nofaralfasi reviewed Feb 22, 2023

View reviewed changes

ekohl requested changes Feb 22, 2023

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Feb 22, 2023

ofedoren force-pushed the feat-36015-add-role-by-one branch from 8434de4 to c7b4e6a Compare March 30, 2023 12:50

theforeman-bot added Needs re-review and removed Waiting on contributor labels Mar 30, 2023

ekohl reviewed Mar 30, 2023

View reviewed changes

ofedoren force-pushed the feat-36015-add-role-by-one branch 2 times, most recently from e6eddf4 to c1d6084 Compare March 31, 2023 12:59

ofedoren force-pushed the feat-36015-add-role-by-one branch from c1d6084 to e34fca1 Compare April 11, 2023 11:50

Fixes #36015 - API endpoints to add/remove single roles

ecc2007

ofedoren force-pushed the feat-36015-add-role-by-one branch from e34fca1 to ecc2007 Compare April 17, 2023 11:50

Ron-Lavi requested a review from nofaralfasi April 30, 2023 15:05

nofaralfasi approved these changes May 3, 2023

View reviewed changes

theforeman-bot removed the Needs re-review label May 3, 2023

nofaralfasi merged commit f347fbe into theforeman:master May 8, 2023
4 checks passed

Gauravtalreja1 mentioned this pull request May 25, 2023

Add and remove ansible_role to host/hostgroup SatelliteQE/nailgun#927

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36015 - API endpoints to add/remove single roles #597

Fixes #36015 - API endpoints to add/remove single roles #597

ofedoren commented Feb 16, 2023

nofaralfasi Feb 21, 2023

ofedoren Feb 21, 2023 •

edited

nofaralfasi Feb 22, 2023 •

edited

ofedoren Feb 23, 2023

nofaralfasi Feb 23, 2023 •

edited

ekohl left a comment

ekohl Mar 30, 2023

ofedoren Mar 31, 2023

ekohl Mar 31, 2023

ofedoren Apr 17, 2023

ekohl Mar 30, 2023

ofedoren Mar 31, 2023

ekohl Mar 30, 2023

ofedoren Mar 31, 2023

ekohl Mar 30, 2023

ofedoren Mar 31, 2023

ofedoren commented Apr 11, 2023 •

edited

nofaralfasi left a comment

nofaralfasi commented May 8, 2023

Fixes #36015 - API endpoints to add/remove single roles #597

Fixes #36015 - API endpoints to add/remove single roles #597

Conversation

ofedoren commented Feb 16, 2023

Choose a reason for hiding this comment

ofedoren Feb 21, 2023 • edited

Choose a reason for hiding this comment

nofaralfasi Feb 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nofaralfasi Feb 23, 2023 • edited

Choose a reason for hiding this comment

ekohl left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofedoren commented Apr 11, 2023 • edited

nofaralfasi left a comment

Choose a reason for hiding this comment

nofaralfasi commented May 8, 2023

ofedoren Feb 21, 2023 •

edited

nofaralfasi Feb 22, 2023 •

edited

nofaralfasi Feb 23, 2023 •

edited

ofedoren commented Apr 11, 2023 •

edited