Merge pull request #400 from Temikus/nonexistent_und_nil_checks

Fixing empty get() behavior

.rubocop.yml

@@ -6,6 +6,9 @@ Style/ConditionalAssignment:
   SingleLineConditionsOnly: true
   EnforcedStyle: assign_inside_condition
 
+Style/RedundantReturn:
+  Enabled: false
+
 Style/FormatString:
   Enabled: false
 

.travis.yml

@@ -12,9 +12,6 @@ matrix:
     - rvm: 2.5
     - rvm: jruby-head
   allow_failures:
-  # allow 2.5 to fail until Travis CI is fixed.
-  # See travis-ci/travis-ci#8969
-    - rvm: 2.5
     - rvm: jruby-head
 notifications:
   email: false

CHANGELOG.md

@@ -9,7 +9,34 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1
 #### Fixed
 
 - \#419 Locked down fog upstream dependencies to alleviate deprecation warnings
-  until they can be properly dealt with.
+  until they can be properly dealt with. [temikus]
+- \#400 Small `%Collection%.get` and `%Collection%.all` behaviour fixes [temikus]
+  - `Fog::Google::SQL::Instances.get(nil)` no longer returns an invalid 
+    `sql#instancesList` object.
+  - `Fog::Compute::Google::InstanceGroups.get` and `.all` methods now support more than
+    just `:filter` option, fixed `.all` output if zone wasn't provided.
+  - Fix a typo causing `Operations.get(region:REGION)` to fail.
+  - `Fog::Compute::Google::Images.get(IMAGE, PROJECT)`, now returns `nil` if image is not
+    found rather than throwing `Google::Apis::ClientError`.
+
+### Development changes
+
+#### Added
+
+- \#400 Additional test coverage [temikus]
+  - Expanded tests for `%Collection%.get` behavior - scoped requests (e.g. `get(zone:ZONE)`)
+    and their corresponding code paths are now also tested.
+  - Increase `Fog::Compute::Google::Images` integration test coverage.
+  - Unit tests now work without a `~/.fog` config file set up.
+  - Expanded unit test coverage.
+
+#### Changed
+
+- \#400 Refactored most compute `get()` and `all()` methods to common format. [temikus]
+
+#### Fixed
+
+- \#400 Removed the Travis Ruby 2.5 workaround. [temikus]
 
 ## 1.7.1
 

CONTRIBUTING.md

@@ -101,20 +101,26 @@ test:
 Then you can run all the live tests:
 
 ```shell
-$ rake test
+$ bundle exec rake test
 ```
 
 or just one API:
 
 ```
-$ rake test:compute
+$ bundle exec rake test:compute
 ```
 (See `rake -T` for all available tasks)
 
-or just one file:
+or just one test:
 
 ```shell
-$ rake test TEST=test/integration/compute/test_servers.rb TESTOPTS="--name=TestServers#test_bootstrap_ssh_destroy"
+$ bundle exec rake test TESTOPTS="--name=TestServers#test_bootstrap_ssh_destroy"
+```
+
+or a series of tests by name:
+
+```
+$ bundle exec rake test TESTOPTS="--name=/test_nil_get/"
 ```
 
 #### Unit tests
@@ -133,7 +139,7 @@ We're in progress of extending the library with more unit tests and contribution
 #### The transition from `shindo` to Minitest
 
 Previously, [shindo](https://github.com/geemus/shindo) was the primary testing framework. 
-We've started moving away from it, and to Minitest, but some artifacts may remain.
+We've moved away from it, and to Minitest, but some artifacts may remain.
 
 For more information on transition, read [#50](https://github.com/fog/fog-google/issues/50).
 

README.md

@@ -42,8 +42,6 @@ We are always looking for people to improve our code and test coverage, so pleas
 
 ## Pubsub
 
-Note: You **must** have a version of google-api-client > 0.8.5 to use the Pub/Sub API; previous versions will not work.
-
 Fog mostly implements [v1](https://cloud.google.com/pubsub/docs/reference/rest/) of the Google Cloud Pub/Sub API; however some less common API methods are missing. Pull requests for additions would be greatly appreciated.
 
 ## Installation

lib/fog/bin/google.rb

@@ -15,6 +15,8 @@ def class_for(key)
         Fog::Storage::Google
       when :sql
         Fog::Google::SQL
+      when :pubsub
+        Fog::Google::Pubsub
       else
         raise ArgumentError, "Unsupported #{self} service: #{key}"
       end
@@ -33,6 +35,8 @@ def [](service)
                       Fog::Google::Monitoring.new
                     when :sql
                       Fog::Google::SQL.new
+                    when :pubsub
+                      Fog::Google::Pubsub.new
                     when :storage
                       Fog::Logger.warning("Google[:storage] is not recommended, use Storage[:google] for portability")
                       Fog::Storage.new(:provider => "Google")

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

@@ -23,9 +23,15 @@ def all(region: nil, filter: nil, max_results: nil, order_by: nil, page_token: n
           load(data.map(&:to_h))
         end
 
-        def get(identity, region)
-          if address = service.get_address(identity, region).to_h
-            new(address)
+        def get(identity, region = nil)
+          if region
+            address = service.get_address(identity, region).to_h
+            return new(address)
+          elsif identity
+            response = all(:filter => "name eq #{identity}",
+                           :max_results => 1)
+            address = response.first unless response.empty?
+            return address
           end
         rescue ::Google::Apis::ClientError => e
           raise e unless e.status_code == 404

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

@@ -10,8 +10,9 @@ def all(_filters = {})
         end
 
         def get(identity)
-          if backend_service = service.get_backend_service(identity)
-            new(backend_service.to_h)
+          if identity
+            backend_service = service.get_backend_service(identity).to_h
+            return new(backend_service)
           end
         rescue ::Google::Apis::ClientError => e
           raise e unless e.status_code == 404

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

@@ -24,15 +24,14 @@ def all(zone: nil, filter: nil, max_results: nil,
         end
 
         def get(identity, zone = nil)
-          response = nil
           if zone
-            response = service.get_disk_type(identity, zone).to_h
+            disk_type = service.get_disk_type(identity, zone).to_h
+            return new(disk_type)
           else
-            disk_types = all(:filter => "name eq .*#{identity}")
-            response = disk_types.first.attributes unless disk_types.empty?
+            response = all(:filter => "name eq .*#{identity}")
+            disk_type = response.first unless response.empty?
+            return disk_type
           end
-          return nil if response.nil?
-          new(response)
         rescue ::Google::Apis::ClientError => e
           raise e unless e.status_code == 404
           nil

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

@@ -23,10 +23,16 @@ def all(zone: nil, filter: nil, max_results: nil, order_by: nil,
           load(data.map(&:to_h))
         end
 
-        def get(identity, zone)
-          response = service.get_disk(identity, zone)
-          return nil if response.nil?
-          new(response.to_h)
+        def get(identity, zone = nil)
+          if zone
+            disk = service.get_disk(identity, zone).to_h
+            return new(disk)
+          elsif identity
+            response = all(:filter => "name eq #{identity}",
+                           :max_results => 1)
+            disk = response.first unless response.empty?
+            return disk
+          end
         rescue ::Google::Apis::ClientError => e
           raise e unless e.status_code == 404
           nil

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

@@ -10,8 +10,9 @@ def all(opts = {})
         end
 
         def get(identity)
-          if firewall = service.get_firewall(identity)
-            new(firewall.to_h)
+          if identity
+            firewall = service.get_firewall(identity).to_h
+            return new(firewall)
           end
         rescue ::Google::Apis::ClientError => e
           raise e unless e.status_code == 404

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

@@ -4,9 +4,7 @@ class Google
       class ForwardingRules < Fog::Collection
         model Fog::Compute::Google::ForwardingRule
 
-        def all(region: nil, filter: nil, max_results: nil,
-                order_by: nil, page_token: nil)
-
+        def all(region: nil, filter: nil, max_results: nil, order_by: nil, page_token: nil)
           opts = {
             :filter => filter,
             :max_results => max_results,
@@ -15,7 +13,7 @@ def all(region: nil, filter: nil, max_results: nil,
           }
 
           if region
-            data = service.list_forwarding_rules(region, opts).items
+            data = service.list_forwarding_rules(region, opts).items || []
           else
             data = []
             service.list_aggregated_forwarding_rules(opts).items
@@ -29,18 +27,16 @@ def all(region: nil, filter: nil, max_results: nil,
         end
 
         def get(identity, region = nil)
-          response = nil
           if region
-            response = service.get_forwarding_rule(identity, region).to_h
-          else
+            forwarding_rule = service.get_forwarding_rule(identity, region).to_h
+            return new(forwarding_rule)
+          elsif identity
             response = all(
               :filter => "name eq #{identity}", :max_results => 1
-            ).first
-            response = response.attributes unless response.nil?
+            )
+            forwarding_rule = response.first unless response.empty?
+            return forwarding_rule
           end
-
-          return nil if response.nil?
-          new(response)
         end
       end
     end

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

@@ -10,8 +10,9 @@ def all(options = {})
         end
 
         def get(identity)
-          if address = service.get_global_address(identity).to_h
-            new(address)
+          if identity
+            address = service.get_global_address(identity).to_h
+            return new(address)
           end
         rescue ::Google::Apis::ClientError => e
           raise e unless e.status_code == 404

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

@@ -10,8 +10,9 @@ def all(opts = {})
         end
 
         def get(identity)
-          if rule = service.get_global_forwarding_rule(identity).to_h
-            new(rule)
+          if identity
+            rule = service.get_global_forwarding_rule(identity).to_h
+            return new(rule)
           end
         rescue ::Google::Apis::ClientError => e
           raise e unless e.status_code == 404

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

@@ -10,9 +10,10 @@ def all(_filters = {})
         end
 
         def get(identity)
-          response = service.get_http_health_check(identity)
-          return nil if response.nil?
-          new(response.to_h)
+          if identity
+            http_health_check = service.get_http_health_check(identity).to_h
+            return new(http_health_check)
+          end
         rescue ::Google::Apis::ClientError => e
           raise e unless e.status_code == 404
           nil

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

@@ -46,22 +46,33 @@ def current
         end
 
         def get(identity, project = nil)
-          project.nil? ? projects = all_projects : projects = [project]
-          data = nil
-          projects.each do |proj|
+          if project
             begin
-              data = service.get_image(identity, proj).to_h
-              data[:project] = proj
+              image = service.get_image(identity, project).to_h
+              # TODO: Remove response modification - see #405
+              image[:project] = project
+              return new(image)
             rescue ::Google::Apis::ClientError => e
-              next if e.status_code == 404
-              break
-            else
-              break
+              raise e unless e.status_code == 404
+              nil
+            end
+          elsif identity
+            projects = all_projects
+            projects.each do |proj|
+              begin
+                response = service.get_image(identity, proj).to_h
+                # TODO: Remove response modification - see #405
+                response[:project] = proj
+                image = response
+                return new(image)
+              rescue ::Google::Apis::ClientError => e
+                next if e.status_code == 404
+                break
+              end
             end
+            # If nothing is found - return nil
+            nil
           end
-
-          return nil if data.nil?
-          new(data)
         end
 
         def get_from_family(family, project = nil)

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

@@ -12,10 +12,11 @@ def all(zone: nil, filter: nil, max_results: nil,
             :order_by => order_by,
             :page_token => page_token
           }
-          data = []
+
           if zone
-            data += service.list_instance_group_managers(zone, opts).items || []
+            data = service.list_instance_group_managers(zone, opts).items || []
           else
+            data = []
             service.list_aggregated_instance_group_managers(opts).items.each_value do |group|
               data.concat(group.instance_group_managers) if group.instance_group_managers
             end
@@ -26,11 +27,13 @@ def all(zone: nil, filter: nil, max_results: nil,
 
         def get(identity, zone = nil)
           if zone
-            if instance_group_manager = service.get_instance_group_manager(identity, zone)
-              new(instance_group_manager.to_h)
-            end
-          else
-            all(:filter => "name eq .*#{identity}").first
+            instance_group_manager = service.get_instance_group_manager(identity, zone).to_h
+            return new(instance_group_manager)
+          elsif identity
+            response = all(:filter => "name eq .*#{identity}",
+                           :max_results => 1)
+            instance_group_manager = response.first unless response.empty?
+            return instance_group_manager
           end
         rescue ::Google::Apis::ClientError => e
           raise e unless e.status_code == 404