Rename log_rate_limit_per_second to log-rate-limit-per-second

All our archeology seems to indicate that dashes should be preferred in manifests as they are yaml and yaml prefers kebab-case to snake_case. We also uncovered some oddities around setting process level properties on the top level app object. We added tests to document that disk and memory don't show up in the diff when changed on the app instead of the process. We also discovered that proprties like health-check that shouldbe kebab-case were not working at the app level and fixed them to be consistent. Signed-off-by: Rebecca Roberts <[email protected]> Co-authored-by: Rebecca Roberts <[email protected]>
cloudfoundry · Aug 19, 2022 · bb70156 · bb70156
1 parent 5f0da2e
commit bb70156
Show file tree

Hide file tree

Showing 8 changed files with 94 additions and 48 deletions.
diff --git a/app/actions/space_diff_manifest.rb b/app/actions/space_diff_manifest.rb
@@ -14,7 +14,9 @@ class << self
       # rubocop:todo Metrics/CyclomaticComplexity
       def generate_diff(app_manifests, space)
         json_diff = []
-        recognized_top_level_keys = AppManifestMessage.allowed_keys.map(&:to_s)
+
+        recognized_top_level_keys = AppManifestMessage.allowed_keys.map(&:to_s).map(&:dasherize)
+
         app_manifests = normalize_units(app_manifests)
         app_manifests.each_with_index do |manifest_app_hash, index|
           manifest_app_hash = filter_manifest_app_hash(manifest_app_hash)
@@ -99,7 +101,7 @@ def filter_manifest_app_hash(manifest_app_hash)
               'type',
               'command',
               'disk_quota',
-              'log_rate_limit_per_second',
+              'log-rate-limit-per-second',
               'health-check-http-endpoint',
               'health-check-invocation-timeout',
               'health-check-type',
@@ -161,7 +163,7 @@ def normalize_units(manifest_app_hash)
           end
         end
 
-        byte_measurement_key_words = ['log_rate_limit_per_second']
+        byte_measurement_key_words = ['log-rate-limit-per-second']
         manifest_app_hash.each_with_index do |process_hash, index|
           byte_measurement_key_words.each do |key|
             value = process_hash[key]

diff --git a/app/presenters/v3/app_manifest_presenters/process_properties_presenter.rb b/app/presenters/v3/app_manifest_presenters/process_properties_presenter.rb
@@ -14,7 +14,7 @@ def process_hash(process)
               'instances' => process.instances,
               'memory' => add_units(process.memory),
               'disk_quota' => add_units(process.disk_quota),
-              'log_rate_limit_per_second' => add_units_log_rate_limit(process.log_rate_limit),
+              'log-rate-limit-per-second' => add_units_log_rate_limit(process.log_rate_limit),
               'command' => process.command,
               'health-check-type' => process.health_check_type,
               'health-check-http-endpoint' => process.health_check_http_endpoint,

diff --git a/spec/request/app_manifests_spec.rb b/spec/request/app_manifests_spec.rb
@@ -108,15 +108,15 @@
                   'instances' => process.instances,
                   'memory' => "#{process.memory}M",
                   'disk_quota' => "#{process.disk_quota}M",
-                  'log_rate_limit_per_second' => '1M',
+                  'log-rate-limit-per-second' => '1M',
                   'health-check-type' => process.health_check_type,
                 },
                 {
                   'type' => worker_process.type,
                   'instances' => worker_process.instances,
                   'memory' => "#{worker_process.memory}M",
                   'disk_quota' => "#{worker_process.disk_quota}M",
-                  'log_rate_limit_per_second' => '1M',
+                  'log-rate-limit-per-second' => '1M',
                   'command' => worker_process.command,
                   'health-check-type' => worker_process.health_check_type,
                   'health-check-http-endpoint' => worker_process.health_check_http_endpoint,
@@ -207,15 +207,15 @@
                   'instances' => process.instances,
                   'memory' => "#{process.memory}M",
                   'disk_quota' => "#{process.disk_quota}M",
-                  'log_rate_limit_per_second' => -1,
+                  'log-rate-limit-per-second' => -1,
                   'health-check-type' => process.health_check_type,
                 },
                 {
                   'type' => worker_process.type,
                   'instances' => worker_process.instances,
                   'memory' => "#{worker_process.memory}M",
                   'disk_quota' => "#{worker_process.disk_quota}M",
-                  'log_rate_limit_per_second' => '1M',
+                  'log-rate-limit-per-second' => '1M',
                   'command' => worker_process.command,
                   'health-check-type' => worker_process.health_check_type,
                   'health-check-http-endpoint' => worker_process.health_check_http_endpoint,

diff --git a/spec/request/space_manifests_spec.rb b/spec/request/space_manifests_spec.rb
@@ -27,7 +27,7 @@
             'instances' => 4,
             'memory' => '2048MB',
             'disk_quota' => '1.5GB',
-            'log_rate_limit_per_second' => '1MB',
+            'log-rate-limit-per-second' => '1MB',
             'buildpack' => buildpack.name,
             'stack' => buildpack.stack,
             'command' => 'new-command',
@@ -74,7 +74,7 @@
             'instances' => 3,
             'memory' => '2048MB',
             'disk_quota' => '1.5GB',
-            'log_rate_limit_per_second' => '100KB',
+            'log-rate-limit-per-second' => '100KB',
             'buildpack' => buildpack.name,
             'stack' => buildpack.stack,
             'command' => 'newer-command',
@@ -247,7 +247,7 @@
               'instances' => 4,
               'memory' => '2048MB',
               'disk_quota' => '1.5GB',
-              'log_rate_limit_per_second' => '1GB',
+              'log-rate-limit-per-second' => '1GB',
               'buildpack' => buildpack.name,
               'stack' => buildpack.stack,
               'command' => 'new-command',
@@ -271,7 +271,7 @@
               'instances' => 4,
               'memory' => '2048MB',
               'disk_quota' => '1.5GB',
-              'log_rate_limit_per_second' => '-1B',
+              'log-rate-limit-per-second' => '-1B',
               'buildpack' => buildpack.name,
               'stack' => buildpack.stack,
               'command' => 'new-command',
@@ -467,7 +467,7 @@
            'version' => 1,
            'applications' => [
              { 'name' => app1_model.name,
-               'log_rate_limit_per_second' => -1
+               'log-rate-limit-per-second' => -1
              },
            ]
         }.to_yaml
@@ -521,7 +521,7 @@
               'instances' => 4,
               'memory' => '2048MB',
               'disk_quota' => '1.5GB',
-              'log_rate_limit_per_second' => '300B',
+              'log-rate-limit-per-second' => '300B',
               'buildpack' => buildpack.name,
               'stack' => buildpack.stack,
               'command' => 'new-command',

diff --git a/spec/unit/actions/space_diff_manifest_spec.rb b/spec/unit/actions/space_diff_manifest_spec.rb
@@ -21,15 +21,15 @@ module VCAP::CloudController
                   'instances' => process1.instances,
                   'memory' => '1024M',
                   'disk_quota' => '1024M',
-                  'log_rate_limit_per_second' => '1M',
+                  'log-rate-limit-per-second' => '1M',
                   'health-check-type' =>  process1.health_check_type
                 },
                 {
                   'type' => process2.type,
                   'instances' => process2.instances,
                   'memory' => '1024M',
                   'disk_quota' => '1024M',
-                  'log_rate_limit_per_second' => '1M',
+                  'log-rate-limit-per-second' => '1M',
                   'health-check-type' =>  process2.health_check_type
                 }
               ]
@@ -71,13 +71,13 @@ module VCAP::CloudController
 
       context 'when there are changes in the manifest' do
         before do
-          default_manifest['applications'][0]['random_route'] = true
+          default_manifest['applications'][0]['random-route'] = true
           default_manifest['applications'][0]['stack'] = 'big brother'
         end
 
         it 'returns the correct diff' do
           expect(subject).to match_array([
-            { 'op' => 'add', 'path' => '/applications/0/random_route', 'value' => true },
+            { 'op' => 'add', 'path' => '/applications/0/random-route', 'value' => true },
             { 'op' => 'replace', 'path' => '/applications/0/stack', 'was' => process1.stack.name, 'value' => 'big brother' },
           ])
         end
@@ -266,7 +266,7 @@ module VCAP::CloudController
           before do
             default_manifest['applications'][0]['processes'][0]['memory'] = '1G'
             default_manifest['applications'][0]['processes'][0]['disk_quota'] = '1G'
-            default_manifest['applications'][0]['processes'][0]['log_rate_limit_per_second'] = '1024K'
+            default_manifest['applications'][0]['processes'][0]['log-rate-limit-per-second'] = '1024K'
           end
           it 'returns an empty diff' do
             expect(subject).to eq([])
@@ -277,13 +277,15 @@ module VCAP::CloudController
           before do
             default_manifest['applications'][0]['processes'][0]['memory'] = '2G'
             default_manifest['applications'][0]['processes'][0]['disk_quota'] = '4G'
-            default_manifest['applications'][0]['processes'][0]['log_rate_limit_per_second'] = '2G'
+            default_manifest['applications'][0]['processes'][0]['health-check-type'] = 'process'
+            default_manifest['applications'][0]['processes'][0]['log-rate-limit-per-second'] = '2G'
           end
           it 'returns the diff formatted' do
             expect(subject).to eq([
               { 'op' => 'replace', 'path' => '/applications/0/processes/0/memory', 'value' => '2048M', 'was' => '1024M' },
               { 'op' => 'replace', 'path' => '/applications/0/processes/0/disk_quota', 'value' => '4096M', 'was' => '1024M' },
-              { 'op' => 'replace', 'path' => '/applications/0/processes/0/log_rate_limit_per_second', 'value' => '2G', 'was' => '1M' },
+              { 'op' => 'replace', 'path' => '/applications/0/processes/0/log-rate-limit-per-second', 'value' => '2G', 'was' => '1M' },
+              { 'op' => 'replace', 'path' => '/applications/0/processes/0/health-check-type', 'value' => 'process', 'was' => 'port' },
             ])
           end
         end
@@ -322,29 +324,71 @@ module VCAP::CloudController
         end
 
         context 'when updating app-level configurations' do
-          before do
-            default_manifest['applications'][0]['memory'] = '1G'
-            default_manifest['applications'][0]['disk_quota'] = '1G'
-            default_manifest['applications'][0]['log_rate_limit_per_second'] = '1024K'
+          context 'when nothing has changed' do
+            before do
+              default_manifest['applications'][0]['log-rate-limit-per-second'] = '1024K'
+            end
+
+            it 'returns an empty diff if the field is equivalent' do
+              expect(subject).to eq([])
+            end
           end
 
-          it 'returns an empty diff if the field is equivalent' do
-            expect(subject).to eq([])
+          context 'when trying to change memory and disk quota' do
+            before do
+              default_manifest['applications'][0]['memory'] = '99G'
+              default_manifest['applications'][0]['disk_quota'] = '99G'
+            end
+
+            it 'returns an empty diff because for some reason we ignore these fields at the app level' do
+              expect(subject).to eq([])
+            end
+          end
+
+          context 'when things have changed' do
+            before do
+              default_manifest['applications'][0]['health-check-type'] = 'process'
+              default_manifest['applications'][0]['instances'] = 9
+              default_manifest['applications'][0]['log-rate-limit-per-second'] = '1G'
+            end
+
+            it 'displays in the diff' do
+              expect(subject).to eq([
+                {
+                  'op' => 'replace',
+                  'path' => '/applications/0/health-check-type',
+                  'was' => 'port',
+                  'value' => 'process'
+                },
+                {
+                  'op' => 'replace',
+                  'path' => '/applications/0/instances',
+                  'was' => 1,
+                  'value' => 9
+                },
+                {
+                  'op' => 'replace',
+                  'path' => '/applications/0/log-rate-limit-per-second',
+                  'was' => '1M',
+                  'value' => '1G'
+                }
+              ])
+            end
           end
         end
       end
 
-      context 'when the log_rate_limit_per_second is unlimited' do
+      context 'when the log-rate-limit-per-second is unlimited' do
         before do
-          default_manifest['applications'][0]['log_rate_limit_per_second'] = -1
+          default_manifest['applications'][0]['log-rate-limit-per-second'] = '-1B'
         end
 
         it 'displays -1 in the diff' do
           expect(subject).to include(
             {
               'op' => 'replace',
-              'path' => '/applications/0/log_rate_limit_per_second',
-              'value' => -1,
+              'path' => '/applications/0/log-rate-limit-per-second',
+              'value' => '-1B',
               'was' => '1M'
             }
           )

diff --git a/spec/unit/messages/app_manifest_message_spec.rb b/spec/unit/messages/app_manifest_message_spec.rb
@@ -104,8 +104,8 @@ module VCAP::CloudController
         end
       end
 
-      describe 'log_rate_limit_per_second' do
-        context 'when log_rate_limit_per_second unit is not part of expected set of values' do
+      describe 'log-rate-limit-per-second' do
+        context 'when log-rate-limit-per-second unit is not part of expected set of values' do
           let(:params_from_yaml) { { name: 'eugene', log_rate_limit_per_second: '200INVALID' } }
 
           it 'is not valid' do
@@ -119,7 +119,7 @@ module VCAP::CloudController
           end
         end
 
-        context 'when log_rate_limit_per_second is less than -1 bytes' do
+        context 'when log-rate-limit-per-second is less than -1 bytes' do
           let(:params_from_yaml) { { name: 'eugene', log_rate_limit_per_second: '-1M' } }
 
           it 'is not valid' do
@@ -131,7 +131,7 @@ module VCAP::CloudController
           end
         end
 
-        context 'when log_rate_limit_per_second is not numeric' do
+        context 'when log-rate-limit-per-second is not numeric' do
           let(:params_from_yaml) { { name: 'eugene', log_rate_limit_per_second: 'gerg herscheisers' } }
 
           it 'is not valid' do
@@ -145,7 +145,7 @@ module VCAP::CloudController
           end
         end
 
-        context 'when log_rate_limit_per_second is an unlimited amount' do
+        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 } }
 
@@ -164,7 +164,7 @@ module VCAP::CloudController
           end
         end
 
-        context 'when log_rate_limit_per_second is in megabytes' do
+        context 'when log-rate-limit-per-second is in megabytes' do
           let(:params_from_yaml) { { name: 'eugene', log_rate_limit_per_second: '1MB' } }
 
           it 'is valid' do
@@ -642,7 +642,7 @@ module VCAP::CloudController
                 'instances' => -30,
                 'memory' => 'potato',
                 'disk_quota' => '100',
-                'log_rate_limit_per_second' => 'kumara',
+                'log-rate-limit-per-second' => 'kumara',
                 'health_check_type' => 'sweet_potato',
                 'health_check_http_endpoint' => '/healthcheck_potato',
                 'health_check_invocation_timeout' => 'yucca',
@@ -656,7 +656,7 @@ module VCAP::CloudController
                 'instances' => 'cassava',
                 'memory' => 'potato',
                 'disk_quota' => '100',
-                'log_rate_limit_per_second' => '100',
+                'log-rate-limit-per-second' => '100',
                 'health_check_type' => 'sweet_potato',
                 'health_check_http_endpoint' => '/healthcheck_potato',
                 'health_check_invocation_timeout' => 'yucca',
@@ -714,18 +714,18 @@ module VCAP::CloudController
           end
         end
 
-        context 'log_rate_limit_per_second' do
-          context 'when log_rate_limit_per_second is an unlimited amount' do
+        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 }] } }
+              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' }] } }
+              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)

diff --git a/spec/unit/presenters/v3/app_manifest_presenter_spec.rb b/spec/unit/presenters/v3/app_manifest_presenter_spec.rb
@@ -120,7 +120,7 @@ module VCAP::CloudController::Presenters::V3
               'type' => process1.type,
               'instances' => process1.instances,
               'memory' => "#{process1.memory}M",
-              'log_rate_limit_per_second' => '1M',
+              'log-rate-limit-per-second' => '1M',
               'disk_quota' => "#{process1.disk_quota}M",
               'command' => process1.command,
               'health-check-type' => process1.health_check_type,
@@ -130,7 +130,7 @@ module VCAP::CloudController::Presenters::V3
             {
               'type' => process2.type,
               'instances' => process2.instances,
-              'log_rate_limit_per_second' => '1M',
+              'log-rate-limit-per-second' => '1M',
               'memory' => "#{process2.memory}M",
               'disk_quota' => "#{process2.disk_quota}M",
               'health-check-type' => process2.health_check_type,
@@ -169,15 +169,15 @@ module VCAP::CloudController::Presenters::V3
               {
                 'type' => process1.type,
                 'instances' => process1.instances,
-                'log_rate_limit_per_second' => '1M',
+                'log-rate-limit-per-second' => '1M',
                 'memory' => "#{process1.memory}M",
                 'disk_quota' => "#{process1.disk_quota}M",
                 'health-check-type' => process1.health_check_type,
               },
               {
                 'type' => process2.type,
                 'instances' => process2.instances,
-                'log_rate_limit_per_second' => '1M',
+                'log-rate-limit-per-second' => '1M',
                 'memory' => "#{process2.memory}M",
                 'disk_quota' => "#{process2.disk_quota}M",
                 'health-check-type' => process2.health_check_type,