Allow specifying buildpacks with CNBs

Author: sethboyles

app/actions/app_apply_manifest.rb

@@ -59,6 +59,7 @@ def apply(app_guid, message)
 
       app_update_message = message.app_update_message
       lifecycle = AppLifecycleProvider.provide_for_update(app_update_message, app)
+
       AppUpdate.new(@user_audit_info, manifest_triggered: true).update(app, app_update_message, lifecycle)
 
       update_routes(app, message)

app/controllers/v3/space_manifests_controller.rb

@@ -16,6 +16,7 @@ def apply_manifest
     can_write_space(space)
 
     messages = parsed_app_manifests.map { |app_manifest| AppManifestMessage.create_from_yml(app_manifest) }
+
     errors = messages.each_with_index.flat_map { |message, i| errors_for_message(message, i) }
     compound_error!(errors) unless errors.empty?
 

app/messages/app_manifest_message.rb

@@ -123,16 +123,32 @@ def audit_hash
     end
 
     def app_lifecycle_hash
-      lifecycle_data = if requested?(:lifecycle) && @lifecycle == 'cnb'
-                         cnb_lifecycle_data
+      lifecycle_type = if requested?(:lifecycle) && @lifecycle == 'cnb'
+                         Lifecycles::CNB
+                       elsif requested?(:lifecycle) && @lifecycle == 'buildpack'
+                         Lifecycles::BUILDPACK
                        elsif requested?(:docker)
-                         docker_lifecycle_data
-                       elsif requested?(:buildpacks) || requested?(:buildpack) || requested?(:stack)
-                         buildpacks_lifecycle_data
+                         Lifecycles::DOCKER
                        end
 
+      data = {
+        buildpacks: requested_buildpacks,
+        stack: @stack,
+        credentials: @cnb_credentials
+      }.compact
+
+      if lifecycle_type == Lifecycles::DOCKER
+        lifecycle = {
+          type: Lifecycles::DOCKER
+        }
+      elsif lifecycle_type || data.present?
+        lifecycle = {}
+        lifecycle[:type] = lifecycle_type if lifecycle_type.present?
+        lifecycle[:data] = data if data.present?
+      end
+
       {
-        lifecycle: lifecycle_data,
+        lifecycle: lifecycle,
         metadata: requested?(:metadata) ? metadata : nil
       }.compact
     end
@@ -279,31 +295,6 @@ def service_bindings_attribute_mapping
       mapping
     end
 
-    def docker_lifecycle_data
-      { type: Lifecycles::DOCKER }
-    end
-
-    def cnb_lifecycle_data
-      {
-        type: Lifecycles::CNB,
-        data: {
-          buildpacks: requested_buildpacks,
-          stack: @stack,
-          credentials: @cnb_credentials
-        }.compact
-      }
-    end
-
-    def buildpacks_lifecycle_data
-      {
-        type: Lifecycles::BUILDPACK,
-        data: {
-          buildpacks: requested_buildpacks,
-          stack: @stack
-        }.compact
-      }
-    end
-
     def should_autodetect?(buildpack)
       buildpack == 'default' || buildpack == 'null' || buildpack.nil?
     end

spec/unit/actions/app_apply_manifest_spec.rb

@@ -147,6 +147,98 @@ module VCAP::CloudController
           end
         end
 
+        context 'cnb apps' do
+          let(:buildpack) { VCAP::CloudController::Buildpack.make }
+          let(:message) { AppManifestMessage.create_from_yml({ name: 'blah' }) }
+          let(:app_update_message) { message.app_update_message }
+          let(:app) { AppModel.make(:cnb) }
+
+          before do
+            TestConfig.override(default_app_lifecycle: 'cnb')
+          end
+
+          context 'when the default_app_lifecycle is set and the the lifecycle is not specified' do
+            it 'preserves the lifecycle' do
+              app_apply_manifest.apply(app.guid, message)
+              expect(AppUpdate).to have_received(:new).with(user_audit_info, manifest_triggered: true)
+              expect(app_update).to have_received(:update).
+                with(app, app_update_message, instance_of(AppCNBLifecycle))
+              expect(app.reload.lifecycle_type).to eq('cnb')
+            end
+          end
+
+          context 'when buildpack is specified' do
+            let(:message) { AppManifestMessage.create_from_yml({ name: 'blah', buildpack: buildpack.name }) }
+
+            it 'preserves the lifecycle' do
+              app_apply_manifest.apply(app.guid, message)
+              expect(AppUpdate).to have_received(:new).with(user_audit_info, manifest_triggered: true)
+              expect(app_update).to have_received(:update).
+                with(app, app_update_message, instance_of(AppCNBLifecycle))
+              expect(app.reload.lifecycle_type).to eq('cnb')
+            end
+          end
+
+          context 'when the default differs from what is already set on the app' do
+            let(:message) { AppManifestMessage.create_from_yml({ name: 'blah', buildpack: buildpack.name }) }
+            let(:app) { AppModel.make(:buildpack) }
+
+            it 'preserves the apps lifecycle' do
+              app_apply_manifest.apply(app.guid, message)
+              expect(AppUpdate).to have_received(:new).with(user_audit_info, manifest_triggered: true)
+              expect(app_update).to have_received(:update).
+                with(app, app_update_message, instance_of(AppBuildpackLifecycle))
+              expect(app.reload.lifecycle_type).to eq('buildpack')
+            end
+          end
+        end
+
+        context 'buildpack apps' do
+          let(:buildpack) { VCAP::CloudController::Buildpack.make }
+          let(:message) { AppManifestMessage.create_from_yml({ name: 'blah' }) }
+          let(:app_update_message) { message.app_update_message }
+          let(:app) { AppModel.make(:buildpack) }
+
+          before do
+            TestConfig.override(default_app_lifecycle: 'buildpack')
+          end
+
+          context 'when the default_app_lifecycle is set and the the lifecycle is not specified' do
+            it 'preserves the lifecycle' do
+              app_apply_manifest.apply(app.guid, message)
+              expect(AppUpdate).to have_received(:new).with(user_audit_info, manifest_triggered: true)
+              expect(app_update).to have_received(:update).
+                with(app, app_update_message, instance_of(AppBuildpackLifecycle))
+              expect(app.reload.lifecycle_type).to eq('buildpack')
+            end
+          end
+
+          context 'when buildpack is specified' do
+            let(:message) { AppManifestMessage.create_from_yml({ name: 'blah', buildpack: buildpack.name }) }
+
+            it 'preserves the lifecycle' do
+              app_apply_manifest.apply(app.guid, message)
+              expect(AppUpdate).to have_received(:new).with(user_audit_info, manifest_triggered: true)
+              expect(app_update).to have_received(:update).
+                with(app, app_update_message, instance_of(AppBuildpackLifecycle))
+              expect(app.reload.lifecycle_type).to eq('buildpack')
+            end
+          end
+
+          context 'when the default differs from what is already set on the app' do
+            let(:message) { AppManifestMessage.create_from_yml({ name: 'blah', buildpack: buildpack.name }) }
+            let(:app) { AppModel.make(:cnb) }
+
+            it 'preserves the apps lifecycle' do
+              app_apply_manifest.apply(app.guid, message)
+              expect(AppUpdate).to have_received(:new).with(user_audit_info, manifest_triggered: true)
+              expect(app_update).to have_received(:update).
+                with(app, app_update_message, instance_of(AppCNBLifecycle))
+              expect(app.reload.lifecycle_type).to eq('cnb')
+            end
+          end
+        end
+
         describe 'updating stack' do
           let(:message) { AppManifestMessage.create_from_yml({ name: 'stack-test', stack: 'cflinuxfs4' }) }
           let(:app_update_message) { message.app_update_message }

spec/unit/jobs/deserialization_spec.rb

@@ -178,7 +178,6 @@ module Jobs
                       - :lifecycle
                       extra_keys: []
                       lifecycle:
-                        :type: buildpack
                         :data:
                           :buildpacks:
                           - ruby

spec/unit/messages/app_manifest_message_spec.rb

@@ -1064,6 +1064,7 @@ module VCAP::CloudController
             message = AppManifestMessage.create_from_yml(params_from_yaml)
 
             expect(message).not_to be_valid
+
             expect(message.errors).to have(1).items
             expect(message.errors.full_messages).to include('Cannot specify both buildpack(s) and docker keys')
           end
@@ -1995,7 +1996,6 @@ module VCAP::CloudController
 
             it 'does not forward missing attributes to the AppUpdateMessage' do
               message = AppManifestMessage.create_from_yml(parsed_yaml)
-
               expect(message.app_update_message.requested?(:lifecycle)).to be false
             end
           end
@@ -2105,6 +2105,42 @@ module VCAP::CloudController
           end
         end
 
+        context 'when buildpack is the default_app_lifecycle but lifecycle is not set' do
+          before do
+            TestConfig.override(default_app_lifecycle: 'buildpack')
+          end
+
+          let(:parsed_yaml) { { name: 'cnb', buildpacks: %w[nodejs java], stack: stack.name } }
+
+          it 'sets the app lifecycle to nil' do
+            message = AppManifestMessage.create_from_yml(parsed_yaml)
+
+            expect(message).to be_valid
+            expect(message.app_update_message.lifecycle_type).to eq(nil)
+            expect(message.app_update_message.buildpack_data.buildpacks).to eq(%w[nodejs java])
+            expect(message.app_update_message.buildpack_data.stack).to eq(stack.name)
+            expect(message.app_update_message.buildpack_data.credentials).to be_nil
+          end
+        end
+
+        context 'when cnb is the default_app_lifecycle but lifecycle is not set' do
+          before do
+            TestConfig.override(default_app_lifecycle: 'cnb')
+          end
+
+          let(:parsed_yaml) { { name: 'cnb', buildpacks: %w[nodejs java], stack: stack.name } }
+
+          it 'sets the app lifecycle to cnb' do
+            message = AppManifestMessage.create_from_yml(parsed_yaml)
+
+            expect(message).to be_valid
+            expect(message.app_update_message.lifecycle_type).to eq(nil)
+            expect(message.app_update_message.buildpack_data.buildpacks).to eq(%w[nodejs java])
+            expect(message.app_update_message.buildpack_data.stack).to eq(stack.name)
+            expect(message.app_update_message.buildpack_data.credentials).to be_nil
+          end
+        end
+
         context 'when cnb is disabled' do
           before do
             FeatureFlag.make(name: 'diego_cnb', enabled: false, error_message: 'I am a banana')