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]>
loggregator · Aug 3, 2022 · a6e3e01 · a6e3e01
1 parent a4de4c2
commit a6e3e01
Show file tree

Hide file tree

Showing 8 changed files with 71 additions and 21 deletions.
diff --git a/app/actions/space_diff_manifest.rb b/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

diff --git a/app/messages/app_manifest_message.rb b/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
 
       byte_converter.convert_to_b(human_readable_byte_value.strip)
     rescue ByteConverter::InvalidUnitsError, ByteConverter::NonNumericError
@@ -392,7 +393,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
@@ -416,7 +419,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

diff --git a/app/presenters/v3/app_manifest_presenters/process_properties_presenter.rb b/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

diff --git a/spec/request/app_manifests_spec.rb b/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,
                 },
                 {

diff --git a/spec/unit/actions/space_diff_manifest_spec.rb b/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) {
           {

diff --git a/spec/unit/messages/app_manifest_message_spec.rb b/spec/unit/messages/app_manifest_message_spec.rb
@@ -127,7 +127,7 @@ module VCAP::CloudController
 
             expect(message).not_to be_valid
             expect(message.errors).to have(1).items
-            expect(message.errors.full_messages).to include('Process "web": Log rate limit must be an integer greater than or equal to -1B')
+            expect(message.errors.full_messages).to include('Process "web": Log rate limit must be an integer greater than or equal to -1')
           end
         end
 
@@ -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

diff --git a/spec/unit/messages/manifest_process_scale_message_spec.rb b/spec/unit/messages/manifest_process_scale_message_spec.rb
@@ -150,7 +150,7 @@ module VCAP::CloudController
 
             expect(message).not_to be_valid
             expect(message.errors.count).to eq(1)
-            expect(message.errors.full_messages).to include('Log rate limit must be an integer greater than or equal to -1B')
+            expect(message.errors.full_messages).to include('Log rate limit must be an integer greater than or equal to -1')
           end
         end
 
@@ -162,7 +162,7 @@ module VCAP::CloudController
 
             expect(message).not_to be_valid
             expect(message.errors.count).to eq(1)
-            expect(message.errors.full_messages).to include('Log rate limit must be an integer greater than or equal to -1B')
+            expect(message.errors.full_messages).to include('Log rate limit must be an integer greater than or equal to -1')
           end
         end
 
@@ -174,7 +174,7 @@ module VCAP::CloudController
 
             expect(message).not_to be_valid
             expect(message.errors.count).to eq(1)
-            expect(message.errors.full_messages).to include('Log rate limit must be an integer greater than or equal to -1B')
+            expect(message.errors.full_messages).to include('Log rate limit must be an integer greater than or equal to -1')
           end
         end
       end

diff --git a/spec/unit/presenters/v3/app_manifest_presenters/process_properties_presenter_spec.rb b/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')