Subnetwork support #216

Wyosotis · 2017-04-24T16:32:01Z

Please review this pull request which has been made for solving the issue #117

houndci-bot · 2017-04-24T16:32:19Z

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

+          end
+
+          network_interface["accessConfigs"] = [access_config] if access_config
+


Trailing whitespace detected.

houndci-bot · 2017-04-24T16:32:19Z

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

+              subnetwork = subnetworks.get(subnetwork, region_name) 
+            end
+
+            network_interface["subnetwork"] = subnetwork.get_self_link_attr() 


Do not use parentheses for method calls with no arguments.
Trailing whitespace detected.

houndci-bot · 2017-04-24T16:32:19Z

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

+              # Extract region_name from zone_name	
+              l = zone_name.split(/-/)
+              region_name = l[0] + "-" + l[1]
+              subnetwork = subnetworks.get(subnetwork, region_name) 


Trailing whitespace detected.

houndci-bot · 2017-04-24T16:32:19Z

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

+
+            # Objectify the subnetwork if needed
+            unless subnetwork.is_a? Subnetwork
+              # Extract region_name from zone_name	


Trailing whitespace detected.

houndci-bot · 2017-04-24T16:32:19Z

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

+
+          if options.key? "subnetwork"
+            subnetwork = options.delete "subnetwork"
+


Trailing whitespace detected.

houndci-bot · 2017-04-24T16:32:19Z

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

@@ -133,8 +133,26 @@ def handle_networks(options)
            network = networks.get(network)
          end

+          network_interface = { "network" => network.get_self_link_attr() }


Do not use parentheses for method calls with no arguments.

houndci-bot · 2017-04-24T16:32:19Z

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

          body["description"] = options["description"] if options["description"]
          network_name = options["network"] ? options["network"].split("/")[-1] : GOOGLE_COMPUTE_DEFAULT_NETWORK
          body["network"] = "https://www.googleapis.com/compute/#{api_version}/projects/#{@project}/global/networks/#{network_name}"

+          unless options["subnetwork"].nil?
+            subnetwork_name = options["subnetwork"].split("/")[-1] 


Trailing whitespace detected.

houndci-bot · 2017-04-24T16:32:19Z

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

@@ -36,6 +36,10 @@ def destroy(async = true)
          operation
        end

+        def get_self_link_attr
+          return [self_link]


Redundant return detected.

houndci-bot · 2017-04-24T16:32:19Z

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

        end
+


Extra empty line detected at class body end.

houndci-bot · 2017-04-24T16:32:19Z

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

-          network_interface["accessConfigs"] = [access_config] if access_config
-          network_interface
+        def get_self_link_attr
+          return [self_link]


Redundant return detected.

houndci-bot · 2017-04-24T16:38:52Z

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

+              region_name = l[0] + "-" + l[1]
+              subnetwork = subnetworks.get(subnetwork, region_name) 
+            end
+            network_interface["subnetwork"] = subnetwork.get_self_link_attr 


Trailing whitespace detected.

houndci-bot · 2017-04-24T16:38:52Z

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

+            subnetwork = options.delete "subnetwork"
+            # Objectify the subnetwork if needed
+            unless subnetwork.is_a? Subnetwork
+              # Extract region_name from zone_name	


Trailing whitespace detected.

You can run rubocop -a filename to autocleanup these.

icco

This looks pretty good so far.

icco · 2017-04-24T20:09:49Z

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

-        def get_as_interface_config(access_config = nil)
-          network_interface = { "network" => self_link }
-          network_interface["accessConfigs"] = [access_config] if access_config
-          network_interface


Is there any way we can leave this method name the same? I'm also not entirely sure what this function is doing (or what it was doing for that matter).

After reading the code that uses this, I'm wondering if we should just delete the function and the one you replaced it with and just put the array creation in the code where it's needed.

I am not sure if it is a good way to add subnetwork setting in the method get_as_interface_config which is the method of class network and not subnetwork. That's why I have moved network_interface outside. I will think about how to make it better.

icco · 2017-04-24T20:59:12Z

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

+            subnetwork = options.delete "subnetwork"
+            # Objectify the subnetwork if needed
+            unless subnetwork.is_a? Subnetwork
+              # Extract region_name from zone_name	


You can run rubocop -a filename to autocleanup these.

icco · 2017-04-24T21:05:06Z

Oh also, can you make sure the network interface are up to date?

Temikus

Thank you very much for this! 🎆

Please, see my comments.

Temikus · 2017-04-25T04:23:55Z

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

@@ -36,12 +36,10 @@ def destroy(async = true)
          operation
        end

-        # Returns a ready API structure for insert_instance, used in insert_server request.
-        def get_as_interface_config(access_config = nil)


Providing some context. The following is my personal opinion, so please, correct me if I'm wrong here.

I implemented this method because I wanted the server insert request to properly work with network models #201, my reasoning for the method was:

Interface config and network resource do not match each-other well

insert_server request was getting overloaded, there were 30 additional lines to the main request method that very awkwardly constructed the network config.

what is better aware of what network looks like than the model itself?

Hence the placement in network model.
I want you to consider this before returning back to generating arrays in the insert code.

Thanks for the context. Finally I have left this method as it was (please see changes in the source code). By the way according to Google API documentation "if the network is not specified but the subnetwork is specified, the network is inferred". It means in the case when a user doesn't mention a network, it should not to be defaulted but to be inferred from a subnetwork. I find this a little bit strange.

@Leila-Kuntar Thank you for this tidbit! I'll file some feedback internally.

Temikus · 2017-04-25T04:39:44Z

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

+            unless subnetwork.is_a? Subnetwork
+              # Extract region_name from zone_name	
+              l = zone_name.split(/-/)
+              region_name = l[0] + "-" + l[1]


Avoid getting options by splitting parameters based on delimiters. Those can change and string manipulation is finicky.

For example, what if they change it? us-central1-f-private for example or us-legacy-central1-f...

I would suggest finding out the region programmatically, something like... get_zone(zone_name).body["region"]
, however, better dedup that with get_zone() in the request, since we're already getting it once, might as well reuse the called object and ingest it in the method.

Nit: I'd also suggest not to instantiate unneeded objects ('l'), unless they are reused so something like zone_name.split("-")[0..1].join("-") would probably be better in my opinion.

Thank you for your comments. I pass region_name as a parameter in handle_networks. Seems to be better now.

Temikus · 2017-04-25T04:51:34Z

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

@@ -27,7 +27,6 @@ def insert_instance_group(group_name, zone, options = {})

          unless options["subnetwork"].nil?
            subnetwork_name = options["subnetwork"].split("/")[-1] 
-            puts subnetwork_name 


Oh wow, thanks for noticing this!

houndci-bot · 2017-04-26T08:27:49Z

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

        def get_as_interface_config(access_config = nil)
          network_interface = { "network" => self_link }
          network_interface["accessConfigs"] = [access_config] if access_config
          network_interface
        end
+


Extra empty line detected at class body end.

houndci-bot · 2017-04-26T08:39:28Z

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

@@ -35,7 +35,7 @@ def destroy(async = true)
          operation.wait_for { ready? } unless async
          operation
        end
-
+        


Trailing whitespace detected.

houndci-bot · 2017-04-26T09:31:25Z

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

@@ -233,14 +243,14 @@ def insert_server(server_name, zone_name, options = {}, *deprecated_args)
                    ohm.upcase == "MIGRATE" && "MIGRATE") || "TERMINATE"
          end
          body_object["scheduling"] = scheduling
-
+          


Trailing whitespace detected.

icco

One small change, but other than that, lgtm. cc @Temikus

icco · 2017-04-27T12:30:55Z

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

@@ -26,6 +26,7 @@ class Server < Fog::Compute::Server
        # These attributes are not available in the representation of an 'instance' returned by the GCE API.
        # They are useful only for the create process
        attribute :network, :aliases => "network"
+        attribute :subnetwork, :aliases => "subnetwork"


You don't need the alias here, neither does network.

icco · 2017-05-07T15:13:17Z

Ping @Temikus

Temikus · 2017-05-09T23:32:02Z

On it, should get to this by Monday.
Want to run some tests to make sure we don't break anything network-related.

EDIT: Changed timeline since I'm swamped.

Temikus · 2017-05-15T06:10:40Z

@Wyosotis PTAL my review comment, workflow is broken for objects. Otherwise LGTM.

Wyosotis · 2017-05-15T14:16:19Z

@Temikus It seems that I miss something. Which review comment do you mean?

Temikus

@Wyosotis PTAL

Looks like I didn't hit the "submit" button on requesting changes, apologies m(_ _)m

Temikus · 2017-05-15T04:44:12Z

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

          end

          # Return a networkInterfaces array
-          [network.get_as_interface_config(access_config)]
+          [network_interfaces]


You're relying on a conditionally-existing variable. If one specifies network as an object and doesn't specify subnetwork, it equals to nil

server = connection.servers.create( :name => "fog-smoke-test-#{Time.now.to_i}", :disks => [disk], :machine_type => "n1-standard-1", :network => connection.networks.get('default'), :private_key_path => File.expand_path("~/.ssh/id_rsa"), :public_key_path => File.expand_path("~/.ssh/id_rsa.pub"), :zone_name => "us-central1-f", :username => ENV["USER"], :tags => ["fog"], :service_accounts => %w(sql-admin bigquery https://www.googleapis.com/auth/compute) )

/Users/temikus/Code/fog-devel/fog-google/lib/fog/google/shared.rb:188:in `build_excon_response': Invalid value for field 'resource': ''. At least one network interface is required. (Fog::Errors::Error) from /Users/temikus/Code/fog-devel/fog-google/lib/fog/google/shared.rb:159:in `request' from /Users/temikus/Code/fog-devel/fog-google/lib/fog/compute/google/requests/insert_server.rb:266:in `insert_server' from /Users/temikus/Code/fog-devel/fog-google/lib/fog/compute/google/models/server.rb:279:in `save' from /Users/temikus/.rbenv/versions/2.1.7/lib/ruby/gems/2.1.0/gems/fog-core-1.43.0/lib/fog/core/collection.rb:51:in `create' from create_instance.rb:21:in `test' from create_instance.rb:59:in `<main>'

This is actually caught by the new integration tests (not saying you should've been aware of them, though, since this is still in progress) - you can use this one to check the compatibility:

λ rake test TEST="test/**/test_compute_networks_collection.rb" (1) (in /Users/temikus/Code/fog-devel/fog-google) Run options: --seed 19937 # Running: E Finished in 24.725542s, 0.0404 runs/s, 0.2022 assertions/s. 1) Error: TestComputeNetworksCollection#test_network_workflow: Fog::Errors::Error: Invalid value for field 'resource': ''. At least one network interface is required. /Users/temikus/Code/fog-devel/fog-google/lib/fog/google/shared.rb:188:in `build_excon_response' /Users/temikus/Code/fog-devel/fog-google/lib/fog/google/shared.rb:159:in `request' /Users/temikus/Code/fog-devel/fog-google/lib/fog/compute/google/requests/insert_server.rb:265:in `insert_server' /Users/temikus/Code/fog-devel/fog-google/lib/fog/compute/google/models/server.rb:279:in `save' /Users/temikus/.rbenv/versions/2.1.7/lib/ruby/gems/2.1.0/gems/fog-core-1.43.0/lib/fog/core/collection.rb:51:in `create' /Users/temikus/Code/fog-devel/fog-google/test/integration/compute/test_compute_networks_collection.rb:39:in `test_network_workflow' 1 runs, 5 assertions, 0 failures, 1 errors, 0 skips rake aborted! Command failed with status (1): [ruby -I"lib:test" -I"/Users/temikus/.rbenv/versions/2.1.7/lib/ruby/gems/2.1.0/gems/rake-11.2.2/lib" "/Users/temikus/.rbenv/versions/2.1.7/lib/ruby/gems/2.1.0/gems/rake-11.2.2/lib/rake/rake_test_loader.rb" "test/integration/compute/test_compute_networks_collection.rb" ] Tasks: TOP => test (See full trace by running task with --trace)

@Temikus or @icco could you tell me please where I should put google_project arguments? I have created the file ~/.fog with credentials as described in README.md but I get the following error

lkuntar@lkuntar-VirtualBox:~/workspace/forks/fog-google$ rake test TEST="test/**/test_compute_networks_collection.rb" Run options: --seed 41630 # Running: E Finished in 0.004641s, 215.4935 runs/s, 0.0000 assertions/s. 1) Error: TestComputeNetworksCollection#test_network_workflow: ArgumentError: Missing required arguments: google_project /var/lib/gems/2.2.0/gems/fog-core-1.43.0/lib/fog/core/service.rb:244:in `validate_options' /var/lib/gems/2.2.0/gems/fog-core-1.43.0/lib/fog/core/service.rb:268:in `handle_settings' /var/lib/gems/2.2.0/gems/fog-core-1.43.0/lib/fog/core/service.rb:98:in `new' /home/lkuntar/workspace/forks/fog-google/test/integration/compute/test_compute_networks_collection.rb:17:in `test_network_workflow' 1 runs, 0 assertions, 0 failures, 1 errors, 0 skips /var/lib/gems/2.2.0/gems/fog-core-1.43.0/lib/fog/core/service.rb:244:in `validate_options': Missing required arguments: google_project (ArgumentError) from /var/lib/gems/2.2.0/gems/fog-core-1.43.0/lib/fog/core/service.rb:268:in `handle_settings' from /var/lib/gems/2.2.0/gems/fog-core-1.43.0/lib/fog/core/service.rb:98:in `new' from /home/lkuntar/workspace/forks/fog-google/test/integration/compute/test_compute_networks_collection.rb:11:in `block in <class:TestComputeNetworksCollection>' from /var/lib/gems/2.2.0/gems/minitest-5.10.1/lib/minitest.rb:58:in `call' from /var/lib/gems/2.2.0/gems/minitest-5.10.1/lib/minitest.rb:58:in `reverse_each' from /var/lib/gems/2.2.0/gems/minitest-5.10.1/lib/minitest.rb:58:in `block (2 levels) in autorun' rake aborted! Command failed with status (1) /var/lib/gems/2.2.0/gems/rake-12.0.0/exe/rake:27:in `<top (required)>' Tasks: TOP => test

@Wyosotis does this help at all? https://github.com/fog/fog-google/blob/master/.fog.example

@Wyosotis Here's mine:

default: google_project: myproject google_json_key_location: /Users/temikus/.gcp/mykey.json public_key_path: ~/.ssh/id_rsa.pub private_key_path: ~/.ssh/id_rsa test: google_project: myproject google_json_key_location: /Users/temikus/.gcp/mykey.json public_key_path: ~/.ssh/id_rsa.pub private_key_path: ~/.ssh/id_rsa

I think the testing suite picks up only "test" credentials, my bad, I'll add that to the example. Thanks for the note!

Thank you for the replies.
Now this test passed well.

lkuntar@lkuntar-VirtualBox:~/workspace/forks/fog-google$ rake test TEST="test/**/test_compute_networks_collection.rb" Run options: --seed 48938 # Running: . Finished in 25.083174s, 0.0399 runs/s, 0.3189 assertions/s. 1 runs, 8 assertions, 0 failures, 0 errors, 0 skips

Temikus · 2017-05-30T02:58:13Z

@Wyosotis Thank you for your work!

LGTM

Wyosotis added 5 commits April 24, 2017 17:56

Add subnetworks support

e571aa8

delete debug info

271ec06

delete hardcoded region-name

b49cca5

replace a tab with two spaces

f89ee99

delete tabs

6ff71e4

houndci-bot reviewed Apr 24, 2017

View reviewed changes

Update insert_server.rb

4d9447a

houndci-bot reviewed Apr 24, 2017

View reviewed changes

Wyosotis added 2 commits April 24, 2017 18:39

Update subnetwork.rb

5d0dc83

Update network.rb

2459e29

icco reviewed Apr 24, 2017

View reviewed changes

Temikus requested changes Apr 25, 2017

View reviewed changes

Temikus reviewed Apr 25, 2017

View reviewed changes

Wyosotis added 2 commits April 25, 2017 19:56

changes due to review comments

7d68d1d

changes due to review comments

99b1b3e

houndci-bot reviewed Apr 26, 2017

View reviewed changes

Wyosotis added 3 commits April 26, 2017 10:30

Update network.rb

f57b4fc

Update network.rb

d63a689

Update network.rb

05d4263

houndci-bot reviewed Apr 26, 2017

View reviewed changes

Wyosotis added 3 commits April 26, 2017 10:46

Update network.rb

5bb0ed2

Update insert_server.rb

f7404d1

Update insert_server.rb

e27dd34

houndci-bot reviewed Apr 26, 2017

View reviewed changes

Update insert_server.rb

b5c16d8

icco approved these changes Apr 27, 2017

View reviewed changes

Temikus requested changes May 16, 2017

View reviewed changes

changes due to networking tests

534abf1

Temikus approved these changes May 30, 2017

View reviewed changes

Temikus merged commit f2d510c into fog:master May 30, 2017

		end

		network_interface["accessConfigs"] = [access_config] if access_config


		if options.key? "subnetwork"
		subnetwork = options.delete "subnetwork"

Subnetwork support #216

Subnetwork support #216

Conversation

Wyosotis commented Apr 24, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icco Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icco commented Apr 24, 2017

Temikus left a comment • edited Loading

Choose a reason for hiding this comment

Temikus Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Temikus Apr 25, 2017 • 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

icco left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icco commented May 7, 2017

Temikus commented May 9, 2017 • edited Loading

Temikus commented May 15, 2017

Wyosotis commented May 15, 2017

Temikus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wyosotis May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Temikus May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Wyosotis May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Temikus commented May 30, 2017

icco Apr 24, 2017 •

edited

Loading

Temikus left a comment •

edited

Loading

Temikus Apr 25, 2017 •

edited

Loading

Temikus Apr 25, 2017 •

edited

Loading

icco left a comment •

edited

Loading

Temikus commented May 9, 2017 •

edited

Loading

Wyosotis May 24, 2017 •

edited

Loading

Temikus May 25, 2017 •

edited

Loading

Wyosotis May 26, 2017 •

edited

Loading