Added PxeServer create, update and delete actions #594

Hyperkid123 · 2019-05-20T13:32:36Z

We've got a BZ for add/edit PxeServer in ui-classic which will require us to rewrite the UI. Part of that is removing current code from ui-classic and let the API handle these actions instead.

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

POST
/api/pxe_servers {
  uri: string,
  name: string,
  pxe_directory?: string,
  windows_images_directory?: string,
  customization_directory?: string
  pxe_menus?: [{
    file_name: string
  }],
  authentication?: { //only if uri has not nfs prefix
    userid: string,
    passworkd: string
  }
}

TODO

~~file depot credentials validation id model: [WIP] Add validation to model for FileDepots manageiq#18795~~

Hyperkid123 · 2019-05-20T13:32:46Z

cc @lpichler

Hyperkid123 · 2019-05-21T13:34:39Z

@miq-bot assign @lpichler

lpichler · 2019-06-10T14:00:50Z

you need to call verify_uri_prefix_before_save.

martinpovolny · 2019-06-11T10:40:39Z

@Hyperkid123 : please, link the BZ for visibility

martinpovolny · 2019-06-13T09:04:04Z

Ping @lpichler

lpichler · 2019-06-13T16:02:54Z

app/controllers/api/pxe_servers_controller.rb

+      menus.each do |menu|
+        validate_pxe_create_data(menu, REQUIRED_PXE_MENU_ATTRS, 'pxe menu')
+      end
+      menus.map do |menu|


this should work also

collection_class(:pxe_menus).create(menu)

in this case, what about to put line ^ on places where create_pxe_menus is called and put validation just before it ?

if menus server.pxe_menus.clear menus.each{|menu| validate_data_for(collection_class(:pxe_menus), menu) } data['pxe_menus'] = collection_class(:pxe_menus).create(menu) end

lpichler · 2019-06-13T16:17:02Z

Validation methods: I see 2 validate_X methods -what about to do 1 method which would take care of all:
similar MISSING and INVALID as we did in Added create orchestration template service dialog action. #596

def validate_data_for(klass, data)

  klass_attributes = PXE_ATTRIBUTES[klass]


   MISSING_ATTRIBUTES = ...

   INVALID_ATTRIBUTES = ....
end

lpichler · 2019-06-13T16:26:39Z

app/controllers/api/pxe_servers_controller.rb

+
+    def edit_resource(type, id, data)
+      PxeServer.transaction do
+        server = resource_search(id, type, collection_class(:pxe_servers))


are other validations missing here ?

lpichler · 2019-06-13T16:28:41Z

app/controllers/api/pxe_servers_controller.rb

+          server.update_authentication(prepare_credentials(authentication), :save => true)
+        end
+
+        # assign pxe menus if needed


I think that comment like this is not needed

lpichler · 2019-06-13T16:32:06Z

app/controllers/api/pxe_servers_controller.rb

 module Api
  class PxeServersController < BaseController
+    INVALID_PXE_SERVER_ATTRS = %w[id href].freeze # Cannot update or create these
+    REQUIRED_PXE_SERVER_ATTRS = %w[name uri].freeze


as I wrote bellow:
you can pust this to the hash:

PXE_ATTRIBUTES = { PxeServer => %w[name uri], ... }.freeze

lpichler · 2019-06-13T16:33:21Z

app/controllers/api/pxe_servers_controller.rb

+        validate_pxe_create_data(data, REQUIRED_PXE_SERVER_ATTRS, 'pxe server')
+
+        server = collection_class(:pxe_servers).new(data)
+        # generates uir_prefix which is ched if server needs authentication or not


this is good comment 👍 (it is not obvious from verify_uri_prefix_before_save why we need it)

lpichler · 2019-06-13T16:34:29Z

app/controllers/api/pxe_servers_controller.rb

+
+    def create_resource(_type, _id, data = {})
+      PxeServer.transaction do
+        authentication = data.delete('authentication')


is it possible to put code outside of PxeServer.transaction block which is not related to the transaction ? (same for edit)

lpichler · 2019-06-13T16:40:46Z

app/controllers/api/pxe_servers_controller.rb

+    private
+
+    def prepare_credentials(authentication)
+      creds = {:default => {}}


can be this just?

{:default => authentication.compact}

not sure if we need method prepare_credentials in this case

lpichler · 2019-06-13T16:43:22Z

overall looks good 👍 and I added some comments - we can discuss it 👍 👾

Hyperkid123 · 2019-06-17T08:09:41Z

overall looks good and I added some comments - we can discuss it

@lpichler I've addressed most of your feedback but i cannot use the classes as keys in hashes because they are not auto loaded correctly. I used string constants instead.

miq-bot · 2019-06-17T08:18:57Z

Checked commits Hyperkid123/manageiq-api@2647011~...d6a2c9d with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

martinpovolny · 2019-06-17T10:02:13Z

@lpichler I've addressed most of your feedback but i cannot use the classes as keys in hashes because they are not auto loaded correctly. I used string constants instead.

Can that be done in a separate PR?

@Hyperkid123 : can you, please, add some API call examples to the Description?

lpichler · 2019-06-17T16:58:13Z

@djberg96 can you review also please ? thanks!

martinpovolny · 2019-06-26T11:09:46Z

ping @djberg96 , @lpichler can we move forward with this one, please?

lpichler · 2019-06-26T13:03:01Z

spec/requests/pxe_servers_spec.rb

@@ -1,7 +1,13 @@
 RSpec.describe 'PxeServers API' do
-  let!(:pxe_server) { FactoryBot.create(:pxe_server) }
+  let(:pxe_server) do


@Hyperkid123 you can do this:

let(:pxe_server) { FactoryBot.build(:pxe_server).tap { |x| x.update_authentication(:default => {:userid => 'foo', :password => 'bar'}) } }

lpichler · 2019-06-26T16:48:44Z

@Hyperkid123 we need to improve authorisation tied with product features.

we need to add for each action which is new also test case for non authorised user.
as I understand correctly it is about those actions: pxe_server_view , pxe_server_create , pxe_server_view, subcollections: pxe_server_view, pxe_server_create

and eventually you need to add it related blocks in api.yml to the XXXX_actions: as I did it here for edit action:

diff --git a/config/api.yml b/config/api.yml
index 6d6e9cc3..97c12862 100644
--- a/config/api.yml
+++ b/config/api.yml
@@ -2445,6 +2445,9 @@
       :get:
       - :name: read
         :identifier: pxe_server_view
+      :patch:
+      - :name: edit
+        :identifier: pxe_server_edit
   :quotas:
     :description: TenantQuotas
     :options:
diff --git a/spec/requests/pxe_servers_spec.rb b/spec/requests/pxe_servers_spec.rb
index 5166427a..c39b9c6e 100644
--- a/spec/requests/pxe_servers_spec.rb
+++ b/spec/requests/pxe_servers_spec.rb
@@ -1,10 +1,6 @@
 RSpec.describe 'PxeServers API' do
-  let(:pxe_server) do
-    pxe_server = FactoryBot.build(:pxe_server)
-    pxe_server.update_authentication(:default => {:userid => 'foo', :password => 'bar'})
-    pxe_server.save
-    pxe_server
-  end
+  let(:pxe_server) { FactoryBot.build(:pxe_server).tap { |x| x.update_authentication(:default => {:userid => 'foo', :password => 'bar'}) } }
+
   let!(:pxe_image_1) { FactoryBot.create(:pxe_image, :pxe_server => pxe_server) }
   let!(:pxe_image_2) { FactoryBot.create(:pxe_image, :pxe_server => pxe_server) }
   let!(:pxe_menu_1) { FactoryBot.create(:pxe_menu, :pxe_server => pxe_server) }
@@ -187,6 +183,14 @@ RSpec.describe 'PxeServers API' do
       post(url, :params => {:name => 'test server', :uri => 'smb://quax', :authentication => {:userid => 'user'}})
       expect(response).to have_http_status(:bad_request)
     end
+
+    it 'will not ... ' do
+      api_basic_authorize
+      post(url, :params => {:name => 'foo', :uri => 'bar/quax',
+                            :pxe_menus      => [{:file_name => 'menu_1'}, {:file_name => 'menu_2'}],
+                            :authentication => {:userid => 'foo', :password => 'bar' }})
+      expect(response).to have_http_status(:forbidden)
+    end
   end
 
   describe 'patch /api/pxe_servers/:id' do
@@ -200,6 +204,12 @@ RSpec.describe 'PxeServers API' do
       expect(response.parsed_body['uri']).to eq('updated/url')
       expect(PxeServer.find(response.parsed_body['id']).pxe_menus.first[:file_name]).to eq('updated menu')
     end
+
+    it 'will not .. ' do
+      api_basic_authorize
+      patch(url, :params => {:name => 'updated name', :uri => 'updated/url', :pxe_menus => [{:file_name => 'updated menu'}]})
+      expect(response).to have_http_status(:forbidden)
+    end
   end
 
   describe 'delete /api/pxe_servers/:id' do
@@ -211,5 +221,11 @@ RSpec.describe 'PxeServers API' do
       expect(response).to have_http_status(:no_content)
       expect(PxeServer.exists?(pxe_server.id)).to be_falsey
     end
+
+    it 'will not show ...' do
+      api_basic_authorize
+      delete(url)
+      expect(response).to have_http_status(:forbidden)
+    end
   end
 end

lpichler

thanks @Hyperkid123

Hyperkid123 force-pushed the add-pxeserver-crud branch 3 times, most recently from 498dcbc to 9a621e3 Compare May 20, 2019 14:35

miq-bot assigned lpichler May 21, 2019

Hyperkid123 mentioned this pull request May 21, 2019

Migrated PXE servers forms to react ManageIQ/manageiq-ui-classic#5614

Merged

3 tasks

Hyperkid123 changed the title ~~Adde PxeServer create, update and delete actions~~ [WIP] Adde PxeServer create, update and delete actions May 22, 2019

miq-bot added the wip label May 22, 2019

Hyperkid123 force-pushed the add-pxeserver-crud branch 9 times, most recently from e294bfd to 1b34362 Compare June 11, 2019 12:42

Hyperkid123 changed the title ~~[WIP] Adde PxeServer create, update and delete actions~~ Adde PxeServer create, update and delete actions Jun 11, 2019

miq-bot removed the wip label Jun 11, 2019

lpichler reviewed Jun 13, 2019

View reviewed changes

Hyperkid123 added 3 commits June 14, 2019 09:38

Added pxe menu subcollection.

2647011

Added create, update and delete actions to pxe servers.

4f333a8

Fix rubocob warnings.

96aedf6

Hyperkid123 force-pushed the add-pxeserver-crud branch from 1b34362 to e6538f6 Compare June 17, 2019 08:06

Hyperkid123 force-pushed the add-pxeserver-crud branch from e6538f6 to d6a2c9d Compare June 17, 2019 08:11

Hyperkid123 changed the title ~~Adde PxeServer create, update and delete actions~~ Added PxeServer create, update and delete actions Jun 17, 2019

Hyperkid123 force-pushed the add-pxeserver-crud branch from d6a2c9d to eef07dc Compare June 26, 2019 12:02

lpichler reviewed Jun 26, 2019

View reviewed changes

Hyperkid123 force-pushed the add-pxeserver-crud branch 2 times, most recently from b13b3fd to 459ad80 Compare June 26, 2019 15:34

Added pxe server request validation.

021be88

Hyperkid123 force-pushed the add-pxeserver-crud branch from 459ad80 to 021be88 Compare June 27, 2019 09:05

lpichler added enhancement changelog/yes labels Jun 28, 2019

lpichler added this to the Sprint 115 Ending Jul 8, 2019 milestone Jun 28, 2019

lpichler approved these changes Jun 28, 2019

View reviewed changes

lpichler merged commit 2279869 into ManageIQ:master Jun 28, 2019

Hyperkid123 deleted the add-pxeserver-crud branch July 1, 2019 06:46

himdel mentioned this pull request Apr 3, 2020

Form conversion: Compute/Infrastructure/PXE/PXE Servers Add/Edit ManageIQ/manageiq-ui-classic#6908

Closed

Added PxeServer create, update and delete actions #594

Added PxeServer create, update and delete actions #594

Uh oh!

Conversation

Hyperkid123 commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Hyperkid123 commented May 20, 2019

Uh oh!

Hyperkid123 commented May 21, 2019

Uh oh!

lpichler commented Jun 10, 2019

Uh oh!

martinpovolny commented Jun 11, 2019

Uh oh!

martinpovolny commented Jun 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpichler commented Jun 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpichler commented Jun 13, 2019

Uh oh!

Hyperkid123 commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miq-bot commented Jun 17, 2019

Uh oh!

martinpovolny commented Jun 17, 2019

Uh oh!

lpichler commented Jun 17, 2019

Uh oh!

martinpovolny commented Jun 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpichler commented Jun 26, 2019

Uh oh!

lpichler left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Hyperkid123 commented May 20, 2019 •

edited

Loading

Hyperkid123 commented Jun 17, 2019 •

edited

Loading