code review fixes

README.md

@@ -15,7 +15,7 @@ GCE authorization is handled via a service account (or personal account) private
 1. GOOGLE_APPLICATION_CREDENTIALS environment variable eg GOOGLE_APPLICATION_CREDENTIALS=/my/home/directory/my_account_key.json
 
 ### DNS
-DNS is integrated via Google's CloudDNS service. To enable a CloudDNS zone name must be provided in the config (see the example yaml file dns_zone_resource_name)
+DNS is integrated via Google's CloudDNS service. To enable, a CloudDNS zone name must be provided in the config (see the example yaml file dns_zone_resource_name)
 
 An A record is then created in that zone upon instance creation with the VM's internal IP, and deleted when the instance is destroyed.
 
@@ -34,8 +34,8 @@ do not have the pool label, and can be configured to allow a specific list of un
 ### Pre-requisite
 
 - A service account needs to be created and a private json key generated (see usage section)
-- The service account needs given permissions to the project (broad permissions would be compute v1 admin and dns admin). A yaml file is provided that lists the least-privilege permissions needed
-- if using DNS, a DNS zone needs to be created
+- The service account needs to be given permissions to the project (broad permissions would be compute v1 admin and dns admin). A yaml file is provided that lists the least-privilege permissions needed
+- if using DNS, a DNS zone needs to be created in CloudDNS, and configured in the provider's config section with the name of that zone (dns_zone_resource_name). When not specified, the DNS setup and teardown is skipped.
 
 
 ## License

lib/vmpooler/providers/gce.rb

@@ -198,7 +198,7 @@ module Vmpooler
             network_interfaces: [network_interfaces],
             labels: { 'vm' => new_vmname, 'pool' => pool_name, project => nil }
           )
-          # TODO: Maybe this will be needed to set the hostname (usually internal DNS name but in opur case for some reason its nil)
+          # TODO: Maybe this will be needed to set the hostname (usually internal DNS name but in our case for some reason its nil)
           # given_hostname = "#{new_vmname}.#{dns_zone}"
           # client.hostname = given_hostname if given_hostname
 

spec/unit/providers/gce_spec.rb

@@ -11,8 +11,7 @@ describe 'Vmpooler::PoolManager::Provider::Gce' do
   let(:metrics) { Vmpooler::Metrics::DummyStatsd.new }
   let(:poolname) { 'debian-9' }
   let(:provider_options) { { 'param' => 'value' } }
-  # let(:project) { 'vmpooler-test' }
-  let(:project) { 'dio-samuel-dev' }
+  let(:project) { 'vmpooler-test' }
   let(:zone) { 'us-west1-b' }
   let(:config) { YAML.load(<<-EOT
 ---
@@ -25,7 +24,6 @@ describe 'Vmpooler::PoolManager::Provider::Gce' do
     project: '#{project}'
     zone: '#{zone}'
     network_name: global/networks/default
-    # network_name: 'projects/itsysopsnetworking/global/networks/shared1'
 :pools:
   - name: '#{poolname}'
     alias: [ 'mockpool' ]
@@ -34,7 +32,6 @@ describe 'Vmpooler::PoolManager::Provider::Gce' do
     timeout: 10
     ready_ttl: 1440
     provider: 'gce'
-    # subnetwork_name: 'projects/itsysopsnetworking/regions/us-west1/subnetworks/vmpooler-test'
     machine_type: 'zones/#{zone}/machineTypes/e2-micro'
 EOT
     )
@@ -63,31 +60,6 @@ EOT
   end
 
   describe '#manual tests live' do
-    skip 'runs in gce' do
-      puts 'creating'
-      result = subject.create_vm(poolname, vmname)
-      subject.get_vm(poolname, vmname)
-      subject.vms_in_pool(poolname)
-
-      puts 'create snapshot w/ one disk'
-      result = subject.create_snapshot(poolname, vmname, 'sams')
-      puts 'create disk'
-      result = subject.create_disk(poolname, vmname, 10)
-      puts 'create snapshot w/ 2 disks'
-      result = subject.create_snapshot(poolname, vmname, 'sams2')
-      puts 'revert snapshot'
-      result = subject.revert_snapshot(poolname, vmname, 'sams')
-      result = subject.destroy_vm(poolname, vmname)
-    end
-
-    skip 'runs existing' do
-      # result = subject.create_snapshot(poolname, vmname, "sams")
-      # result = subject.revert_snapshot(poolname, vmname, "sams")
-      # puts subject.get_vm(poolname, vmname)
-      result = subject.create_vm(poolname, vmname)
-      result = subject.destroy_vm(poolname, vmname)
-    end
-
     context 'in itsysops' do
       let(:vmname) { "instance-15" }
       let(:project) { 'vmpooler-test' }

vmpooler-provider-gce.gemspec

@@ -17,7 +17,7 @@ Gem::Specification.new do |s|
   s.require_paths = ["lib"]
   s.add_dependency "google-apis-compute_v1", "~> 0.14"
   s.add_dependency "googleauth", "~> 0.16.2"
-  s.add_dependency "google-cloud-dns", "~>0.35.1"
+  s.add_dependency "google-cloud-dns", "~> 0.35.1"
   
   s.add_development_dependency 'vmpooler', '~> 1.3', '>= 1.3.0'