Handle -1 without a byte suffix

- We accept -1 without a byte suffix to mean unlimited
- We also return -1 in the rendered manifest

[#182624538](https://www.pivotaltracker.com/story/show/182624538)
Github Issue: cloudfoundry/capi-release#245

Co-authored-by: Rebecca Roberts <[email protected]>
Co-authored-by: Andrew Crump <[email protected]>

app/actions/space_diff_manifest.rb

@@ -165,7 +165,7 @@ def normalize_units(manifest_app_hash)
         manifest_app_hash.each_with_index do |process_hash, index|
           byte_measurement_key_words.each do |key|
             value = process_hash[key]
-            manifest_app_hash[index][key] = normalize_unit(value, key) unless value.nil?
+            manifest_app_hash[index][key] = normalize_unit(value, key) unless value.nil? || value == -1
           end
         end
         manifest_app_hash

app/messages/app_manifest_message.rb

@@ -292,6 +292,7 @@ def convert_to_mb(human_readable_byte_value)
 
     def convert_to_bytes_per_second(human_readable_byte_value)
       return nil unless human_readable_byte_value.present?
+      return -1 if human_readable_byte_value == -1 # unlimited
 
       human_readable_byte_value = human_readable_byte_value.strip
       byte_converter.convert_to_b(human_readable_byte_value)
@@ -393,7 +394,9 @@ def validate_processes!
         type = process[:type]
         memory_error = validate_byte_format(process[:memory], 'Memory')
         disk_error = validate_byte_format(process[:disk_quota], 'Disk quota')
-        log_rate_limit_error = validate_byte_format(process[:log_rate_limit_per_second], 'Log rate limit per second')
+        if process[:log_rate_limit_per_second] != -1
+          log_rate_limit_error = validate_byte_format(process[:log_rate_limit_per_second], 'Log rate limit per second')
+        end
         add_process_error!(memory_error, type) if memory_error
         add_process_error!(disk_error, type) if disk_error
         add_process_error!(log_rate_limit_error, type) if log_rate_limit_error
@@ -417,7 +420,9 @@ def validate_sidecars!
     def validate_top_level_web_process!
       memory_error = validate_byte_format(memory, 'Memory')
       disk_error = validate_byte_format(disk_quota, 'Disk quota')
-      log_rate_limit_error = validate_byte_format(log_rate_limit_per_second, 'Log rate limit per second')
+      if log_rate_limit_per_second != -1
+        log_rate_limit_error = validate_byte_format(log_rate_limit_per_second, 'Log rate limit per second')
+      end
       add_process_error!(memory_error, ProcessTypes::WEB) if memory_error
       add_process_error!(disk_error, ProcessTypes::WEB) if disk_error
       add_process_error!(log_rate_limit_error, ProcessTypes::WEB) if log_rate_limit_error

app/presenters/v3/app_manifest_presenters/process_properties_presenter.rb

@@ -27,11 +27,9 @@ def add_units(val)
           end
 
           def add_units_log_rate_limit(val)
-            if val == -1
-              '-1B'
-            else
-              byte_converter.human_readable_byte_value(val)
-            end
+            return -1 if val == -1
+
+            byte_converter.human_readable_byte_value(val)
           end
 
           def byte_converter

spec/request/app_manifests_spec.rb

@@ -207,7 +207,7 @@
                   'instances' => process.instances,
                   'memory' => "#{process.memory}M",
                   'disk_quota' => "#{process.disk_quota}M",
-                  'log_rate_limit_per_second' => '-1B',
+                  'log_rate_limit_per_second' => -1,
                   'health-check-type' => process.health_check_type,
                 },
                 {

spec/request/space_manifests_spec.rb

@@ -461,6 +461,33 @@
       end
     end
 
+    context 'when -1 is given as a log rate limit' do
+      let(:yml_manifest) do
+        {
+           'version' => 1,
+           'applications' => [
+             { 'name' => app1_model.name,
+               'log_rate_limit_per_second' => -1
+             },
+           ]
+        }.to_yaml
+      end
+      it 'interprets the log rate limit as unlimited' do
+        post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
+
+        expect(last_response.status).to eq(202)
+        job_guid = VCAP::CloudController::PollableJobModel.last.guid
+        expect(last_response.headers['Location']).to match(%r(/v3/jobs/#{job_guid}))
+
+        Delayed::Worker.new.work_off
+        expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete,
+          VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error
+
+        app1_model.reload
+        expect(app1_model.processes.first.log_rate_limit).to eq(-1)
+      end
+    end
+
     context 'when applying the manifest to an app which is exceeding the log rate limit' do
       before do
         app1_model.web_processes.first.update(state: VCAP::CloudController::ProcessModel::STARTED, instances: 4)

spec/unit/actions/space_diff_manifest_spec.rb

@@ -334,6 +334,23 @@ module VCAP::CloudController
         end
       end
 
+      context 'when the log_rate_limit_per_second is unlimited' do
+        before do
+          default_manifest['applications'][0]['log_rate_limit_per_second'] = -1
+        end
+
+        it 'displays -1 in the diff' do
+          expect(subject).to include(
+            {
+              'op' => 'replace',
+              'path' => '/applications/0/log_rate_limit_per_second',
+              'value' => -1,
+              'was' => '1M'
+            }
+          )
+        end
+      end
+
       context 'when the user passes in a v2 manifest' do
         let(:default_manifest) {
           {

spec/unit/messages/app_manifest_message_spec.rb

@@ -145,12 +145,22 @@ module VCAP::CloudController
           end
         end
 
-        context 'when log_rate_limit_per_second is unlimited amount' do
-          let(:params_from_yaml) { { name: 'eugene', log_rate_limit_per_second: '-1B' } }
+        context 'when log_rate_limit_per_second is an unlimited amount' do
+          context 'specified as -1' do
+            let(:params_from_yaml) { { name: 'eugene', log_rate_limit_per_second: -1 } }
 
-          it 'is valid' do
-            message = AppManifestMessage.create_from_yml(params_from_yaml)
-            expect(message).to be_valid
+            it 'is valid' do
+              message = AppManifestMessage.create_from_yml(params_from_yaml)
+              expect(message).to be_valid
+            end
+          end
+          context 'with a bytes suffix' do
+            let(:params_from_yaml) { { name: 'eugene', log_rate_limit_per_second: '-1B' } }
+
+            it 'is valid' do
+              message = AppManifestMessage.create_from_yml(params_from_yaml)
+              expect(message).to be_valid
+            end
           end
         end
 
@@ -162,7 +172,6 @@ module VCAP::CloudController
             expect(message).to be_valid
           end
         end
-
       end
 
       describe 'buildpack' do
@@ -704,6 +713,27 @@ module VCAP::CloudController
             expect(message.errors.full_messages).to include('Process "bob" may only be present once')
           end
         end
+
+        context 'log_rate_limit_per_second' do
+          context 'when log_rate_limit_per_second is an unlimited amount' do
+            context 'specified as -1' do
+              let(:params_from_yaml) { { name: 'eugene', processes: [{ 'type' => 'foo', 'log_rate_limit_per_second' => -1 }] } }
+
+              it 'is valid' do
+                message = AppManifestMessage.create_from_yml(params_from_yaml)
+                expect(message).to be_valid
+              end
+            end
+            context 'with a bytes suffix' do
+              let(:params_from_yaml) { { name: 'eugene', processes: [{ 'type' => 'foo', 'log_rate_limit_per_second' => '-1B' }] } }
+
+              it 'is valid' do
+                message = AppManifestMessage.create_from_yml(params_from_yaml)
+                expect(message).to be_valid
+              end
+            end
+          end
+        end
       end
 
       describe 'sidecars' do

spec/unit/presenters/v3/app_manifest_presenters/process_properties_presenter_spec.rb

@@ -50,8 +50,8 @@ module VCAP::CloudController::Presenters::V3::AppManifestPresenters
     end
 
     describe '#add_units_log_rate_limit' do
-      it 'is consistant with other quotas with output' do
-        expect(subject.add_units_log_rate_limit(-1)).to eq('-1B')
+      it 'is consistent with other quotas with output' do
+        expect(subject.add_units_log_rate_limit(-1)).to eq(-1)
         expect(subject.add_units_log_rate_limit(256)).to eq('256B')
         expect(subject.add_units_log_rate_limit(2_048)).to eq('2K')
         expect(subject.add_units_log_rate_limit(4_194_304)).to eq('4M')