Add templates and instance group managers #348

bpaquet · 2018-05-29T01:51:35Z

Sorry, did not write tests, but I'm using it in prod.

houndci-bot · 2018-05-29T01:51:56Z

lib/fog/compute/google/requests/set_instance_template.rb

+  module Compute
+    class Google
+      class Mock
+        def set_instance_template(instance_group_manager, instance_template)


Lint/UnusedMethodArgument: Unused method argument - instance_group_manager. If it's necessary, use _ or _instance_group_manager as an argument name to indicate that it won't be used. You can also write as set_instance_template() if you want the method to accept any arguments but don't care about them.
Lint/UnusedMethodArgument: Unused method argument - instance_template. If it's necessary, use _ or _instance_template as an argument name to indicate that it won't be used. You can also write as set_instance_template() if you want the method to accept any arguments but don't care about them.

houndci-bot · 2018-05-29T01:51:56Z

lib/fog/compute/google/requests/recreate_instances.rb

+      class Real
+        def recreate_instances(instance_group_manager, instances)
+          request = ::Google::Apis::ComputeV1::InstanceGroupManagersAbandonInstancesRequest.new(
+            :instances => instances.map{|x| x.self_link}


Layout/SpaceBeforeBlockBraces: Space missing to the left of {.
Layout/SpaceInsideBlockBraces: Space between { and | missing.
Style/SymbolProc: Pass &:self_link as an argument to map instead of a block.
Layout/SpaceInsideBlockBraces: Space missing inside }.

houndci-bot · 2018-05-29T01:51:56Z

lib/fog/compute/google/requests/recreate_instances.rb

+  module Compute
+    class Google
+      class Mock
+        def recreate_instances(instance_group_manager, instances)


Lint/UnusedMethodArgument: Unused method argument - instance_group_manager. If it's necessary, use _ or _instance_group_manager as an argument name to indicate that it won't be used. You can also write as recreate_instances() if you want the method to accept any arguments but don't care about them.
Lint/UnusedMethodArgument: Unused method argument - instances. If it's necessary, use _ or _instances as an argument name to indicate that it won't be used. You can also write as recreate_instances() if you want the method to accept any arguments but don't care about them.

houndci-bot · 2018-05-29T01:51:56Z

lib/fog/compute/google/requests/list_instance_templates.rb

+    class Google
+
+      class Mock
+        def list_instance_templates()


Style/DefWithParentheses: Omit the parentheses in defs when the method doesn't accept any arguments.

houndci-bot · 2018-05-29T01:51:56Z

lib/fog/compute/google/requests/list_instance_templates.rb

+module Fog
+  module Compute
+    class Google
+


Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body beginning.

houndci-bot · 2018-05-29T01:51:57Z

lib/fog/compute/google/models/instance_template.rb

+          end
+          operation
+        end
+


Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body end.

houndci-bot · 2018-05-29T01:51:58Z

lib/fog/compute/google/models/instance_group_managers.rb

+
+          load(data.map(&:to_h))
+        end
+


Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body end.

houndci-bot · 2018-05-29T01:51:58Z

lib/fog/compute/google/models/instance_group_manager.rb

+        def abandon_instances instances
+          service.abandon_instances self, instances
+        end
+


Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body end.

houndci-bot · 2018-05-29T01:51:58Z

lib/fog/compute/google/models/instance_group_manager.rb

+          service.recreate_instances self, instances
+        end
+
+        def abandon_instances instances


Style/MethodDefParentheses: Use def with parentheses when there are parameters.

houndci-bot · 2018-05-29T01:51:58Z

lib/fog/compute/google/models/instance_group_manager.rb

+          service.set_instance_template self, instance_template
+        end
+
+        def recreate_instances instances


Style/MethodDefParentheses: Use def with parentheses when there are parameters.

houndci-bot · 2018-05-29T01:57:50Z

lib/fog/compute/google/models/instance_group_manager.rb

+        attribute :instance_group, :aliases => "instanceGroup"
+        attribute :instance_template, :aliases => "instanceTemplate"
+
+        def set_instance_template instance_template


Style/MethodDefParentheses: Use def with parentheses when there are parameters.

icco · 2018-05-30T19:53:23Z

lib/fog/compute/google/models/instance_group_managers.rb

+
+        def all(filters = {})
+          if filters[:zone]
+            data = Array(service.list_aggregated_instance_group_managers(filters[:zone]))


Why do you need to cast this as an Array? What does it return normally?

icco · 2018-05-30T19:54:52Z

lib/fog/compute/google/models/instance_group_managers.rb

+      class InstanceGroupManagers < Fog::Collection
+        model Fog::Compute::Google::InstanceGroupManager
+
+        def all(filters = {})


data = [] should be the first line in this function to properly declare scope.

icco · 2018-05-30T19:56:35Z

lib/fog/compute/google/models/instance_group_managers.rb

+            data = Array(service.list_aggregated_instance_group_managers(filters[:zone]))
+          else
+            data = []
+            service.list_aggregated_instance_group_managers.items.each_value do |group|


I believe this could be simplified to

data = service.list_aggregated_instance_group_managers.items.map {|group| group.instance_group_managers }

This is not written like that in others files, but yes, we can do that.

icco · 2018-05-30T19:59:35Z

Also, feel free to ignore those codeclimate errors.

houndci-bot · 2018-06-01T04:51:18Z

lib/fog/compute/google/requests/list_instance_group_managers.rb

+
+      class Real
+        def list_instance_group_managers(zone)
+           @compute.list_instance_group_managers(@project, zone)


Layout/IndentationWidth: Use 2 (not 3) spaces for indentation.

Temikus · 2018-06-03T03:06:02Z

First of all - thank you!

Let me write in a quick test for this and I’ll check if this fits our standard model/collection lifecycle.

I aim to respond by Wednesday (SYD) at the latest.

icco

Looks good from a code perspective. Just need @Temikus and we're good to go.

Temikus · 2018-06-04T00:21:51Z

Ok, added a simple lifecycle test:

diff --git a/test/integration/compute/test_instance_group_managers.rb b/test/integration/compute/test_instance_group_managers.rb
new file mode 100644
index 0000000..aea91ef
--- /dev/null
+++ b/test/integration/compute/test_instance_group_managers.rb
@@ -0,0 +1,11 @@
+require "helpers/integration_test_helper"
+require "integration/factories/instance_group_manager_factory"
+
+class TestInstanceGroupManager < FogIntegrationTest
+  include TestCollection
+
+  def setup
+    @subject = Fog::Compute[:google].instance_group_managers
+    @factory = InstanceGroupManagerFactory.new(namespaced_name)
+  end
+end
diff --git a/test/integration/factories/instance_group_manager_factory.rb b/test/integration/factories/instance_group_manager_factory.rb
new file mode 100644
index 0000000..58bb13d
--- /dev/null
+++ b/test/integration/factories/instance_group_manager_factory.rb
@@ -0,0 +1,11 @@
+require "integration/factories/collection_factory"
+
+class InstanceGroupManagerFactory < CollectionFactory
+  def initialize(example)
+    super(Fog::Compute[:google].instance_group_managers, example)
+  end
+
+  def params
+    {:name => resource_name}
+  end
+end

Results:

  1) Error:
TestInstanceGroupManager#test_get_returns_nil_if_resource_does_not_exist:
NoMethodError: undefined method `get' for #<Fog::Compute::Google::InstanceGroupManagers:0x00007ff6b2021c68>
Did you mean?  gem
    /Users/temikus/Dropbox/Code/fog-google/test/integration/factories/collection_factory.rb:21:in `get'
    /Users/temikus/Dropbox/Code/fog-google/test/helpers/test_collection.rb:27:in `test_get_returns_nil_if_resource_does_not_exist'

  2) Error:
TestInstanceGroupManager#test_lifecycle:
NoMethodError: undefined method `save' for #<Fog::Compute::Google::InstanceGroupManager:0x00007ff6b01114b8>
    /Users/temikus/Dropbox/Code/fog-google/test/helpers/test_collection.rb:8:in `test_lifecycle'

We're missing a .save method for InstanceGroupManager model and .get method for InstanceGroupManagers collection.

Temikus

Overall - good work, thank you!

However, needs some adjustments, esp. to the :properties parameter as it's unclear how they should look like and the :properties do not accept hashes.

Temikus · 2018-06-04T00:23:01Z

lib/fog/compute/google/models/instance_group_managers.rb

+  module Compute
+    class Google
+      class InstanceGroupManagers < Fog::Collection
+        model Fog::Compute::Google::InstanceGroupManager


This is missing a get method.

Temikus · 2018-06-04T00:23:45Z

lib/fog/compute/google/models/instance_group_manager.rb

+module Fog
+  module Compute
+    class Google
+      class InstanceGroupManager < Fog::Model


This is missing a save method.

Done. Also added a bunch of attributes; reload and destroy.

Temikus · 2018-06-04T00:49:18Z

lib/fog/compute/google/requests/insert_instance_template.rb

+          instance_template = ::Google::Apis::ComputeV1::InstanceTemplate.new(
+            :description => description,
+            :name => name,
+            :properties => properties,


If I'm reading this correctly, properties will need to be an instance of google api client's InstanceProperties class.

If so - fog avoids exposing underlying API structures, so parameters need to be provided in hashes, see: https://github.com/fog/fog-google/blob/985cd021bcfe4f403deb7c0077997529d529b4b1/lib/fog/compute/google/requests/insert_server.rb

A hash seeme to be enough. Added some doc.

Temikus · 2018-06-04T00:50:47Z

lib/fog/compute/google/requests/insert_instance_template.rb

+      end
+
+      class Real
+        def insert_instance_template(name, properties, description)


Since it's not entirely clear what properties should contain and in what structure, can you please add a doc comment?

E.g.:

fog-google/lib/fog/compute/google/requests/insert_server.rb

Lines 38 to 60 in 985cd02

# Create a new instance (virtual machine).

#

# This method allows you to use low-level request options and thus

# expects instance options similar to API requests. If you don't need to

# modify low-level request options, consider using the

# Fog::Compute::Google::Servers collection object instead.

#

# @example minimal server creation

# my_operation = client.insert_server(

# "my-server",

# "us-central1-a",

# :machine_type => "f1-micro",

# :disks => [

# {

# :initialize_params => {

# :source_image => "projects/debian-cloud/global/images/family/debian-8"

# }

# }

# ]

# )

#

# @param instance_name [String]

# Name to assign to the created server. Must be unique within the specified zone.

Temikus · 2018-06-04T00:55:51Z

lib/fog/compute/google/models/instance_templates.rb

+          if instance_template = service.get_instance_template(identity)
+            new(instance_template.to_h)
+          end
+        rescue Fog::Errors::NotFound


You should rescue Google::Apis::ClientError: notFound, this currently doesn't return nil as it should.

For an example see:

fog-google/lib/fog/compute/google/models/servers.rb

Lines 29 to 42 in 985cd02

def get(identity, zone = nil)

response = nil

if zone

response = service.get_server(identity, zone).to_h

else

server = all(:filter => "name eq .*#{identity}").first

response = server.attributes if server

end

return nil if response.nil?

new(response)

rescue ::Google::Apis::ClientError => e

raise e unless e.status_code == 404

nil

end

Temikus · 2018-06-04T01:04:10Z

@bpaquet to make things easier I've published the quick sanity tests I wrote:
#350

houndci-bot · 2018-06-05T05:30:52Z

lib/fog/compute/google/requests/insert_instance_template.rb

+      end
+
+      class Real
+


Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body beginning.

houndci-bot · 2018-06-05T05:30:53Z

lib/fog/compute/google/requests/insert_instance_group_manager.rb

+
+      class Real
+        def insert_instance_group_manager(name, zone, instance_template, base_instance_name,
+          target_size, target_pools, named_ports, description)


Layout/AlignParameters: Align the parameters of a method definition if they span more than one line.

houndci-bot · 2018-06-05T05:30:53Z

lib/fog/compute/google/requests/insert_instance_group_manager.rb

+    class Google
+      class Mock
+        def insert_instance_group_manager(_name, _zone, _instance_template, _base_instance_name,
+          _target_size, _target_pools, _named_ports, _description)


Layout/AlignParameters: Align the parameters of a method definition if they span more than one line.

houndci-bot · 2018-06-05T05:30:53Z

lib/fog/compute/google/models/instance_group_manager.rb

+          requires :name, :zone, :base_instance_name, :target_size, :instance_template
+
+          data = service.insert_instance_group_manager(name, zone.split("/")[-1], instance_template, base_instance_name,
+          target_size, target_pools, named_ports, description)


Layout/AlignParameters: Align the parameters of a method call if they span more than one line.

houndci-bot · 2018-06-05T05:30:53Z

lib/fog/compute/google/requests/insert_instance_template.rb

+      end
+
+      class Real
+


Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body beginning.

houndci-bot · 2018-06-05T05:30:53Z

lib/fog/compute/google/requests/insert_instance_group_manager.rb

+
+      class Real
+        def insert_instance_group_manager(name, zone, instance_template, base_instance_name,
+          target_size, target_pools, named_ports, description)


Layout/AlignParameters: Align the parameters of a method definition if they span more than one line.

houndci-bot · 2018-06-05T05:30:53Z

lib/fog/compute/google/requests/insert_instance_group_manager.rb

+    class Google
+      class Mock
+        def insert_instance_group_manager(_name, _zone, _instance_template, _base_instance_name,
+          _target_size, _target_pools, _named_ports, _description)


Layout/AlignParameters: Align the parameters of a method definition if they span more than one line.

houndci-bot · 2018-06-05T05:30:54Z

lib/fog/compute/google/models/instance_group_manager.rb

+          requires :name, :zone, :base_instance_name, :target_size, :instance_template
+
+          data = service.insert_instance_group_manager(name, zone.split("/")[-1], instance_template, base_instance_name,
+          target_size, target_pools, named_ports, description)


Layout/AlignParameters: Align the parameters of a method call if they span more than one line.

Temikus

Found some other issues that prevent proper model lifecycle, PTAL

Apologies for yet another iteration m(_ _)m

Temikus · 2018-06-05T22:01:13Z

lib/fog/compute/google/models/instance_group_managers.rb

+          load(data.map(&:to_h))
+        end
+
+        def get(identity, zone)


get should have at most one required argument.

See servers for an example:

fog-google/lib/fog/compute/google/models/servers.rb

Lines 30 to 40 in a142a85

def get(identity, zone = nil)

response = nil

if zone

response = service.get_server(identity, zone).to_h

else

server = all(:filter => "name eq .*#{identity}").first

response = server.attributes if server

end

return nil if response.nil?

new(response)

rescue ::Google::Apis::ClientError => e

I do realise disks have a zone requirement but that's not by design and will be fixed.

Temikus · 2018-06-06T10:55:24Z

lib/fog/compute/google/models/instance_templates.rb

+
+        def all
+          data = service.list_instance_templates.items
+          load(data.map(&:to_h))


Use safe navigation or define as empty array, this will fail if there are no instance templates created.
I.e.: NoMethodError: undefined method map' for nil:NilClass`

This should work:

data = service.list_instance_templates.items || []

Temikus · 2018-06-06T11:31:22Z

lib/fog/compute/google/models/instance_template.rb

+
+        def destroy(async = true)
+          requires :name
+          operation = service.delete_instance_template(name)


You need to return a Fog::Compute::Google::Operation model here, see your save method.
Otherwise you'll get a Google API client class that doesn't respond to .wait_for:

NoMethodError: undefined method `wait_for' for #<Google::Apis::ComputeV1::Operation:0x00007fd00aed3f00>

Temikus · 2018-06-06T11:47:53Z

lib/fog/compute/google/requests/insert_instance_group_manager.rb

+          instance_group_manager = ::Google::Apis::ComputeV1::InstanceGroupManager.new(
+            :description => description,
+            :name => name,
+            :instance_template => instance_template.self_link,


Nit: This assumes that an object is going to be passed in.
Usually models should accept self_links and resource names as well.

Done in the three requests.

Temikus · 2018-06-06T12:01:35Z

lib/fog/compute/google/models/instance_template.rb

+        attribute :self_link, :aliases => "selfLink"
+        attribute :description
+        # Properties is a hash describing the templates
+        # @see https://cloud.google.com/compute/docs/reference/rest/v1/instanceTemplates/insert


Nit: If at all possible, add a minimal hash example. The doc itself is quite complicated and it took me a while to figure the right parameters out (didn't work with IGM api before). For newer people that haven't worked with Google API's at all that may be a higher barrier.

I came up with this as a minimal test case:

:properties => { :machine_type => TEST_MACHINE_TYPE, :disks => [{ :boot => true, :initialize_params => {:source_image => "projects/ubuntu-os-cloud/global/images/ubuntu-1804-bionic-v20180522"} }], :network_interfaces => [{ :network => "global/networks/default" }] }

, but wanted to check with you since you can probably provide a better real-life example.

Done.

No, I'm using template to clone existing template and change the image.

houndci-bot · 2018-06-07T04:49:19Z

lib/fog/compute/google/requests/list_instance_group_managers.rb

+                                         order_by: nil, page_token: nil)
+          @compute.list_instance_group_managers(
+            @project, zone, :filter => filter, :max_results => max_results,
+            :order_by => order_by, :page_token => page_token)


Layout/AlignHash: Align the elements of a hash literal if they span more than one line.
Layout/MultilineMethodCallBraceLayout: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

Temikus

Tests pass \o/
I'm happy how this turned out. Thank you very much for your contribution and patience!

houndci-bot reviewed May 29, 2018

View reviewed changes

bpaquet force-pushed the templates branch from d1bb218 to 53aed8b Compare May 29, 2018 01:57

houndci-bot reviewed May 29, 2018

View reviewed changes

bpaquet force-pushed the templates branch 3 times, most recently from 20974d8 to 6e23342 Compare May 29, 2018 02:04

icco reviewed May 30, 2018

View reviewed changes

bpaquet force-pushed the templates branch from 6e23342 to 824b692 Compare June 1, 2018 04:50

houndci-bot reviewed Jun 1, 2018

View reviewed changes

bpaquet force-pushed the templates branch from 824b692 to 979e2ec Compare June 1, 2018 04:52

icco approved these changes Jun 3, 2018

View reviewed changes

Temikus self-requested a review June 4, 2018 00:22

Temikus requested changes Jun 4, 2018

View reviewed changes

Temikus mentioned this pull request Jun 5, 2018

Add IGM/InstanceTemplate tests #350

Merged

bpaquet force-pushed the templates branch 2 times, most recently from 7848ae9 to 65bc4b3 Compare June 5, 2018 02:23

houndci-bot reviewed Jun 5, 2018

View reviewed changes

Temikus mentioned this pull request Jun 6, 2018

Adding unit tests #353

Merged

bpaquet force-pushed the templates branch from 65bc4b3 to 0c17469 Compare June 6, 2018 01:46

Temikus requested changes Jun 6, 2018

View reviewed changes

bpaquet force-pushed the templates branch from 0c17469 to eaa97a3 Compare June 7, 2018 04:49

houndci-bot reviewed Jun 7, 2018

View reviewed changes

Add templates and instance group managers

893538d

bpaquet force-pushed the templates branch from eaa97a3 to 893538d Compare June 7, 2018 04:50

Temikus approved these changes Jun 8, 2018

View reviewed changes

Temikus merged commit def7ce3 into fog:master Jun 8, 2018

	# Create a new instance (virtual machine).
	#
	# This method allows you to use low-level request options and thus
	# expects instance options similar to API requests. If you don't need to
	# modify low-level request options, consider using the
	# Fog::Compute::Google::Servers collection object instead.
	#
	# @example minimal server creation
	# my_operation = client.insert_server(
	# "my-server",
	# "us-central1-a",
	# :machine_type => "f1-micro",
	# :disks => [
	# {
	# :initialize_params => {
	# :source_image => "projects/debian-cloud/global/images/family/debian-8"
	# }
	# }
	# ]
	# )
	#
	# @param instance_name [String]
	# Name to assign to the created server. Must be unique within the specified zone.

	def get(identity, zone = nil)
	response = nil
	if zone
	response = service.get_server(identity, zone).to_h
	else
	server = all(:filter => "name eq .*#{identity}").first
	response = server.attributes if server
	end
	return nil if response.nil?
	new(response)
	rescue ::Google::Apis::ClientError => e
	raise e unless e.status_code == 404
	nil
	end

Add templates and instance group managers #348

Add templates and instance group managers #348

Conversation

bpaquet commented May 29, 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

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

icco commented May 30, 2018

Choose a reason for hiding this comment

Temikus commented Jun 3, 2018

icco left a comment • edited Loading

Choose a reason for hiding this comment

Temikus commented Jun 4, 2018

Temikus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Temikus Jun 4, 2018 • 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

Temikus commented Jun 4, 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

Temikus 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

Temikus left a comment

Choose a reason for hiding this comment

icco left a comment •

edited

Loading

Temikus Jun 4, 2018 •

edited

Loading