api: copy orchestration template #13053

cynepco3hahue · 2016-12-08T13:52:51Z

For the SSUI we need to have possibility to copy OrchestrationTemplate via API

jntullo · 2016-12-08T19:58:14Z

spec/requests/api/orchestration_template_spec.rb

+
+      expect do
+        run_post(orchestration_templates_url(test_ot.id), gen_request(:copy, :content => ''))
+      end.to change(OrchestrationTemplateCfn, :count).by(2)


should test the http status and expected response as well here

jntullo · 2016-12-08T20:05:01Z

app/models/orchestration_template.rb

@@ -144,6 +144,18 @@ def save_as_orderable!
    new_record? ? replace_with_old_template(old_template) : transfer_stacks(old_template)
  end

+  def deep_copy(new_attributes = {})


not really sure this deep copy is needed since it's only dup-ing and updating the attributes

jntullo · 2016-12-08T20:06:29Z

spec/requests/api/orchestration_template_spec.rb

+      api_basic_authorize collection_action_identifier(:orchestration_templates, :copy)
+
+      run_post(orchestration_templates_url(test_ot.id), gen_request(:copy))
+      expect(response.status).to eq(400)


just a suggestion, but this could also be written
expect(response).to have_http_status(:forbidden) which is nice for readability 😄

jntullo · 2016-12-08T20:14:09Z

spec/requests/api/orchestration_template_spec.rb

@@ -125,4 +125,23 @@
      expect(OrchestrationTemplate.exists?(hot.id)).to be_falsey
    end
  end
+
+  context 'orchestration template copy' do


should also add a test for copying multiple

@cynepco3hahue can you also add tests for unhappy path for authorization for this feature?

jntullo · 2016-12-13T14:37:56Z

app/controllers/api/base_controller/generic.rb

@@ -94,6 +94,23 @@ def delete_resource(type, id = nil, _data = nil)
        delete_resource_action(klass, type, id)
      end

+      def copy_resource(type, id, data = {})


This should be in app/controllers/api/orchestration_template_controller.rb because it is specific to orchestration templates

It looks pretty general, so I thought maybe it can be useful in the future if you want to add copy API for some entity.
What do you think?

Not a bad idea - but that's a question for @abellotti or @imtayadeway 😃

@cynepco3hahue I would make the same recommendation as per @jntullo - if it is needed for another resource we can always review that later, and there may be a better way of sharing that between API controllers without giving it to everyone in the base class

jntullo · 2016-12-13T14:39:42Z

app/controllers/api/base_controller/generic.rb

+      def copy_resource(type, id, data = {})
+        klass    = collection_class(type)
+        resource = resource_search(id, type, klass)
+        attributes = data.dup


don't think you need to dup the data here since you're not manipulating it

jntullo · 2016-12-13T14:49:04Z

app/controllers/api/base_controller/generic.rb

+          if new_resource.respond_to? "#{attr}="
+            new_resource.send("#{attr}=", value)
+          else
+            raise ActiveModel::UnknownAttributeError.new(self, attr)


would not raise an UnknownAttributeError since that'll be handled by the rescue below. Would just do a raise "uknown attribute #{attr}"

jntullo · 2016-12-13T15:03:56Z

app/controllers/api/base_controller/generic.rb

+        end
+        new_resource.tap(&:save!)
+      rescue => err
+        raise BadRequestError, "Failed to copy orchestration template - #{err}"


If it stays in generic, will need to remove reference to orchestration template

imtayadeway · 2016-12-14T15:31:07Z

app/controllers/api/orchestration_templates_controller.rb

+      klass    = collection_class(type)
+      resource = resource_search(id, type, klass)
+      new_resource = resource.dup
+      data.each do |attr, value|


Does new_resource.assign_attributes(data) achieve the same effect?

yes, thanks now it looks better

imtayadeway · 2016-12-14T16:44:38Z

spec/requests/api/orchestration_template_spec.rb

+          orchestration_templates_url(test_ot_1.id),
+          gen_request(:copy, :content => test_content_1)
+        )
+      end.to change(OrchestrationTemplateCfn, :count).by(2)


Why did it change by 2? Is it because the execution is where the lazy-evaluated test_ot_1 gets created? For this reason I think it's better to avoid lets and create everything you are going to test in the setup phase in the body of the test. Duplication is OK in tests and can improve readability

I will change it.

imtayadeway · 2016-12-14T16:48:36Z

spec/requests/api/orchestration_template_spec.rb

+      expect do
+        run_post(
+          orchestration_templates_url(test_ot_1.id),
+          gen_request(:copy, :content => test_content_1)


This test doesn't make clear the value of this feature. Can you already exercise this behavior by creating an orchestration template (via POST/create) with the attributes specified?

I believe it will save you from entering a new name, description and type. Also, we have the possibility to copy an orchestration template via UI, so it better to have such possibility via API.
I believe @chriskacerguis can explain the intention after this feature in the context of self-service.

@cynepco3hahue that's fine, but you are essentially only exercising behavior we already have through POST/create in this test - can you update to verify the copying of existing attributes works?

oh sure I will add it

imtayadeway · 2016-12-14T16:50:02Z

app/controllers/api/orchestration_templates_controller.rb

+          raise "uknown attribute #{attr}"
+        end
+      end
+      new_resource.tap(&:save!)


Is the tap necessary?

yes if you want to return the object

imtayadeway · 2016-12-14T18:03:38Z

spec/requests/api/orchestration_template_spec.rb

+      api_basic_authorize collection_action_identifier(:orchestration_templates, :copy)
+
+      expected = {
+        'content' => test_content_1


Can you verify that you don't get the id/href back of the original?

I will add the verification for id, but adding verification for the href a little bit problematic, FactorGirl OrchestrationEntity does not have the href.
I believe to check only id's good enough for test purpose.
What do you think?

imtayadeway · 2016-12-15T15:34:05Z

spec/requests/api/orchestration_template_spec.rb

@@ -125,4 +125,70 @@
      expect(OrchestrationTemplate.exists?(hot.id)).to be_falsey
    end
  end
+
+  context 'orchestration template copy' do
+    it 'copy template with the same content' do


Can you make these fully descriptive and full sentences (including the "it") so something like "it will not copy a template with no attributes specified" or similar...

imtayadeway · 2016-12-15T15:36:41Z

spec/requests/api/orchestration_template_spec.rb

+
+  context 'orchestration template copy' do
+    it 'copy template with the same content' do
+      test_ot_1 = FactoryGirl.create(:orchestration_template_cfn)


It should be clear that this is a test.... perhaps simply "orchestration_template" or "orchestration_template_1" for these variables?

imtayadeway · 2016-12-15T15:37:37Z

spec/requests/api/orchestration_template_spec.rb

+    it 'copy template with the same content' do
+      test_ot_1 = FactoryGirl.create(:orchestration_template_cfn)
+
+      api_basic_authorize collection_action_identifier(:orchestration_templates, :copy)


To be consistent with spacing can you try to group lines that belong to each phase of the test, i.e. all setup steps together, all execution steps, all verification steps, etc...

imtayadeway · 2016-12-15T15:41:33Z

spec/requests/api/orchestration_template_spec.rb

+
+    it 'copy single template with the different content' do
+      test_ot_1 = FactoryGirl.create(:orchestration_template_cfn)
+      test_content_1 = "{ 'Description': 'Test content 1' }\n"


I'm curious as to why this doesn't get deserialized by Rails when you call response.parsed_body?

I think it because we add new line in the orchestration template by intention(I can see it from code)
text.encode(:universal_newline => true).chomp.concat("\n")

imtayadeway · 2016-12-15T16:54:39Z

spec/requests/api/orchestration_template_spec.rb

@@ -125,4 +125,83 @@
      expect(OrchestrationTemplate.exists?(hot.id)).to be_falsey
    end
  end
+
+  context 'orchestration template copy' do
+    before do


befores, like lets, are best for details which are necessary but not relevant to understanding the behavior being exercised. Since both these objects are central, I'd resist the urge to DRY up here and allow a little duplication for the sake of readability

imtayadeway · 2016-12-15T16:56:11Z

spec/requests/api/orchestration_template_spec.rb

+      api_basic_authorize collection_action_identifier(:orchestration_templates, :copy)
+
+      orchestration_template_2 = FactoryGirl.create(:orchestration_template_cfn)
+      new_content_2 = "{ 'Description': 'Test content 2' }\n"


Again, I'm curious as to why this doesn't get parsed/deserialized. Does { "description" => "Test content 2" } work?

I think it because we add new line in the orchestration template by intention(I can see it from code)
text.encode(:universal_newline => true).chomp.concat("\n")

Could this cause issues with consuming/parsing this API? @abellotti any thoughts here?

cynepco3hahue · 2016-12-22T13:37:00Z

@imtayadeway @jntullo Can you please review again?

cynepco3hahue · 2017-01-03T14:26:36Z

@imtayadeway @jntullo Can you please review again?

jntullo · 2017-01-03T15:05:30Z

app/controllers/api/orchestration_templates_controller.rb

+    def copy_resource(type, id, data = {})
+      klass    = collection_class(type)
+      resource = resource_search(id, type, klass)
+      new_resource = resource.dup


rather than using .tap(&:save!) below, you can just tap the whole thing.

resource.dup.tap do |new_resource| new_resource.assign_attributes(data) new_resource.save! end

abellotti

minor changes requested, otherwise I'm 👍 with this. Thanks.

abellotti · 2017-01-04T17:00:25Z

app/controllers/api/orchestration_templates_controller.rb

+
+    def copy_resource(type, id, data = {})
+      klass    = collection_class(type)
+      resource = resource_search(id, type, klass)


can combine the above 2 lines,

resource = resource_search(id, type, collection_class(type))

abellotti · 2017-01-04T17:03:29Z

app/controllers/api/orchestration_templates_controller.rb

+        new_resource.save!
+      end
+    rescue => err
+      raise BadRequestError, "Failed to copy #{klass} - #{err}"


would prefer english instead of klass/type.

... "Failed to copy orchestration template - #{err}"

imtayadeway · 2017-01-04T17:57:16Z

@cynepco3hahue can you take care of pending requests above? Thanks!

For the SSUI we need to have possibility to copy OrchestrationTemplate via API

miq-bot · 2017-01-04T19:32:38Z

Checked commit cynepco3hahue@d3aeb20 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍰

abellotti · 2017-01-04T20:25:26Z

LGTM!!

chessbyte added api enhancement labels Dec 8, 2016

chessbyte assigned abellotti Dec 8, 2016

jntullo reviewed Dec 8, 2016

View reviewed changes

cynepco3hahue force-pushed the API_copy_ot branch 4 times, most recently from 9798578 to 6db0c32 Compare December 13, 2016 08:44

jntullo reviewed Dec 13, 2016

View reviewed changes

cynepco3hahue force-pushed the API_copy_ot branch 2 times, most recently from eed9c95 to bf7c315 Compare December 14, 2016 06:50

imtayadeway reviewed Dec 14, 2016

View reviewed changes

cynepco3hahue force-pushed the API_copy_ot branch from bf7c315 to f65199c Compare December 14, 2016 18:00

imtayadeway reviewed Dec 14, 2016

View reviewed changes

cynepco3hahue force-pushed the API_copy_ot branch from f65199c to 0d92e65 Compare December 14, 2016 18:29

imtayadeway reviewed Dec 15, 2016

View reviewed changes

cynepco3hahue force-pushed the API_copy_ot branch from 0d92e65 to 067204e Compare December 15, 2016 16:41

imtayadeway reviewed Dec 15, 2016

View reviewed changes

cynepco3hahue force-pushed the API_copy_ot branch from 067204e to 8206426 Compare December 18, 2016 12:47

jntullo reviewed Jan 3, 2017

View reviewed changes

cynepco3hahue force-pushed the API_copy_ot branch from 8206426 to 139fc1b Compare January 4, 2017 07:04

abellotti reviewed Jan 4, 2017

View reviewed changes

api: copy orchestration template

d3aeb20

For the SSUI we need to have possibility to copy OrchestrationTemplate via API

cynepco3hahue force-pushed the API_copy_ot branch from 139fc1b to d3aeb20 Compare January 4, 2017 19:31

abellotti merged commit 0b19f50 into ManageIQ:master Jan 4, 2017

abellotti added this to the Sprint 52 Ending Jan 16, 2017 milestone Jan 4, 2017

cynepco3hahue deleted the API_copy_ot branch January 18, 2017 15:24

api: copy orchestration template #13053

api: copy orchestration template #13053

Conversation

cynepco3hahue commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jntullo Dec 8, 2016 • edited Loading

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

jntullo Dec 13, 2016 • edited Loading

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

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

Choose a reason for hiding this comment

cynepco3hahue commented Dec 22, 2016

cynepco3hahue commented Jan 3, 2017

Choose a reason for hiding this comment

abellotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtayadeway commented Jan 4, 2017

miq-bot commented Jan 4, 2017

abellotti commented Jan 4, 2017

jntullo Dec 8, 2016 •

edited

Loading

jntullo Dec 13, 2016 •

edited

Loading