Install buildpacks based on order on config

- Account for existing system buildpacks by offsetting the install position
- Skip index for buildpacks with explicit position in config

resolves #3798

Author: tomkennedy513

app/jobs/runtime/buildpack_installer_factory.rb

@@ -56,7 +56,8 @@ def plan(buildpack_name, manifest_fields)
                                                                                                    name: buildpack_name,
                                                                                                    stack: buildpack_fields[:stack],
                                                                                                    file: buildpack_fields[:file],
-                                                                                                   options: buildpack_fields[:options]
+                                                                                                   options: buildpack_fields[:options],
+                                                                                                   config_index: buildpack_fields[:config_index]
                                                                                                  })
                             end
           end

app/jobs/runtime/create_buildpack_installer.rb

@@ -2,6 +2,13 @@ module VCAP::CloudController
   module Jobs
     module Runtime
       class CreateBuildpackInstaller < BuildpackInstaller
+        attr_accessor :config_index
+
+        def initialize(job_options)
+          super
+          @config_index = job_options[:config_index]
+        end
+
         def perform
           logger.info "Installing buildpack name `#{name}' with stack `#{stack_name}'"
 
@@ -11,6 +18,8 @@ def perform
           buildpacks_lock.db.transaction do
             buildpacks_lock.lock!
             buildpack = Buildpack.create(name: name, stack: stack_name, lifecycle: options[:lifecycle])
+            # config_index is pre-calculated to account for existing buildpacks, so we can use it directly
+            buildpack.move_to(config_index + 1) if !config_index.nil? && !options.key?(:position)
           end
           begin
             buildpack_uploader.upload_buildpack(buildpack, file, File.basename(file))

lib/cloud_controller/install_buildpacks.rb

@@ -12,34 +12,15 @@ def install(buildpacks)
       CloudController::DependencyLocator.instance.buildpack_blobstore.ensure_bucket_exists
       job_factory = VCAP::CloudController::Jobs::Runtime::BuildpackInstallerFactory.new
 
+      existing_count_by_lifecycle = {
+        Lifecycles::BUILDPACK => Buildpack.where(lifecycle: Lifecycles::BUILDPACK).count,
+        Lifecycles::CNB => Buildpack.where(lifecycle: Lifecycles::CNB).count
+      }
+
       factory_options = []
       buildpacks.each do |bpack|
-        buildpack_opts = bpack.deep_symbolize_keys
-
-        buildpack_opts[:lifecycle] = Lifecycles::BUILDPACK if buildpack_opts[:lifecycle].nil?
-
-        buildpack_name = buildpack_opts.delete(:name)
-        if buildpack_name.nil?
-          logger.error "A name must be specified for the buildpack_opts: #{buildpack_opts}"
-          next
-        end
-
-        package = buildpack_opts.delete(:package)
-        buildpack_file = buildpack_opts.delete(:file)
-        if package.nil? && buildpack_file.nil?
-          logger.error "A package or file must be specified for the buildpack_opts: #{bpack}"
-          next
-        end
-
-        buildpack_file = buildpack_zip(package, buildpack_file)
-        if buildpack_file.nil?
-          logger.error "No file found for the buildpack_opts: #{bpack}"
-          next
-        elsif !File.file?(buildpack_file)
-          logger.error "File not found: #{buildpack_file}, for the buildpack_opts: #{bpack}"
-          next
-        end
-        factory_options << { name: buildpack_name, file: buildpack_file, options: buildpack_opts, stack: detected_stack(buildpack_file, buildpack_opts) }
+        opts = buildpack_factory_options(bpack, existing_count_by_lifecycle, factory_options)
+        factory_options << opts if opts
       end
 
       buildpack_install_jobs = generate_install_jobs(factory_options, job_factory)
@@ -55,6 +36,45 @@ def logger
 
     private
 
+    def buildpack_factory_options(bpack, existing_count_by_lifecycle, factory_options)
+      buildpack_opts = bpack.deep_symbolize_keys
+      buildpack_opts[:lifecycle] = Lifecycles::BUILDPACK if buildpack_opts[:lifecycle].nil?
+
+      buildpack_name = buildpack_opts.delete(:name)
+      if buildpack_name.nil?
+        logger.error "A name must be specified for the buildpack_opts: #{buildpack_opts}"
+        return
+      end
+
+      package = buildpack_opts.delete(:package)
+      buildpack_file = buildpack_opts.delete(:file)
+      if package.nil? && buildpack_file.nil?
+        logger.error "A package or file must be specified for the buildpack_opts: #{bpack}"
+        return
+      end
+
+      buildpack_file = buildpack_zip(package, buildpack_file)
+      if buildpack_file.nil?
+        logger.error "No file found for the buildpack_opts: #{bpack}"
+        return
+      elsif !File.file?(buildpack_file)
+        logger.error "File not found: #{buildpack_file}, for the buildpack_opts: #{bpack}"
+        return
+      end
+
+      lifecycle = buildpack_opts[:lifecycle]
+      existing_offset = existing_count_by_lifecycle[lifecycle] || 0
+      # exclude counting buildpacks with position because they don't need a config index
+      unless buildpack_opts.key?(:position)
+        config_index = existing_offset + factory_options.count do |opts|
+          opts[:options][:lifecycle] == lifecycle && !opts[:options].key?(:position)
+        end
+      end
+
+      { name: buildpack_name, file: buildpack_file, options: buildpack_opts, stack: detected_stack(buildpack_file, buildpack_opts),
+        config_index: config_index }
+    end
+
     def generate_install_jobs(factory_options, job_factory)
       buildpack_install_jobs = []
       buildpacks_by_lifecycle = factory_options.group_by { |options| options[:options][:lifecycle] }

spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb

@@ -15,7 +15,7 @@ module Jobs::Runtime
         end
 
         context 'when the manifest has one buildpack' do
-          let(:buildpack_fields) { [{ file: file, options: opts }] }
+          let(:buildpack_fields) { [{ file: file, options: opts, config_index: 0 }] }
           let(:single_buildpack_job) { jobs.first }
 
           shared_examples_for 'passthrough parameters' do
@@ -34,7 +34,7 @@ module Jobs::Runtime
 
           context 'when there is no matching buildpack record by name' do
             context 'and there is a detected stack in the zipfile' do
-              let(:buildpack_fields) { [{ file: file, options: opts, stack: 'detected stack' }] }
+              let(:buildpack_fields) { [{ file: file, options: opts, stack: 'detected stack', config_index: 0 }] }
 
               include_examples 'passthrough parameters'
 
@@ -45,6 +45,18 @@ module Jobs::Runtime
               it 'sets the stack to the detected stack' do
                 expect(single_buildpack_job.stack_name).to eq('detected stack')
               end
+
+              it 'passes config_index to the create job' do
+                expect(single_buildpack_job.config_index).to eq(0)
+              end
+
+              context 'with a specific config_index' do
+                let(:buildpack_fields) { [{ file: file, options: opts, stack: 'detected stack', config_index: 3 }] }
+
+                it 'passes the config_index through' do
+                  expect(single_buildpack_job.config_index).to eq(3)
+                end
+              end
             end
 
             context 'and there is not a detected stack in the zipfile' do
@@ -66,7 +78,7 @@ module Jobs::Runtime
               let!(:existing_buildpack) { Buildpack.make(name: name, stack: existing_stack.name, key: 'new_key', guid: 'the-guid') }
 
               context 'and the buildpack zip has the same stack' do
-                let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name }] }
+                let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name, config_index: 0 }] }
 
                 include_examples 'passthrough parameters'
 
@@ -84,7 +96,7 @@ module Jobs::Runtime
               end
 
               context 'and the buildpack zip has a different stack' do
-                let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack' }] }
+                let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack', config_index: 0 }] }
 
                 include_examples 'passthrough parameters'
 
@@ -98,7 +110,7 @@ module Jobs::Runtime
               end
 
               context 'and the manifest stack is nil' do
-                let(:buildpack_fields) { [{ file: file, options: opts, stack: nil }] }
+                let(:buildpack_fields) { [{ file: file, options: opts, stack: nil, config_index: 0 }] }
 
                 it 'errors' do
                   expect do
@@ -112,7 +124,7 @@ module Jobs::Runtime
               let!(:existing_buildpack) { Buildpack.make(name: name, stack: nil, key: 'new_key', guid: 'the-guid') }
 
               context 'and the buildpack zip also has a nil stack' do
-                let(:buildpack_fields) { [{ file: file, options: opts, stack: nil }] }
+                let(:buildpack_fields) { [{ file: file, options: opts, stack: nil, config_index: 0 }] }
 
                 include_examples 'passthrough parameters'
 
@@ -130,7 +142,7 @@ module Jobs::Runtime
               end
 
               context 'but the buildpack zip /has/ a stack' do
-                let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack' }] }
+                let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack', config_index: 0 }] }
 
                 include_examples 'passthrough parameters'
 
@@ -157,7 +169,7 @@ module Jobs::Runtime
             let!(:another_existing_buildpack) { Buildpack.make(name: name, stack: another_existing_stack.name, key: 'new_key', guid: 'another-guid') }
 
             context 'and one matches the manifest stack' do
-              let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name }] }
+              let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name, config_index: 0 }] }
 
               include_examples 'passthrough parameters'
 
@@ -175,7 +187,7 @@ module Jobs::Runtime
             end
 
             context 'and the manifest stack is nil' do
-              let(:buildpack_fields) { [{ file: file, options: opts, stack: nil }] }
+              let(:buildpack_fields) { [{ file: file, options: opts, stack: nil, config_index: 0 }] }
 
               it 'errors' do
                 expect do
@@ -185,7 +197,7 @@ module Jobs::Runtime
             end
 
             context 'and none match the manifest stack' do
-              let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack' }] }
+              let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack', config_index: 0 }] }
 
               include_examples 'passthrough parameters'
 
@@ -202,7 +214,9 @@ module Jobs::Runtime
 
         context 'when the manifest has multiple buildpack entries for one name, with different stacks' do
           let(:another_file) { 'the-other-file' }
-          let(:buildpack_fields) { [{ file: file, options: opts, stack: 'existing stack' }, { file: another_file, options: opts, stack: 'manifest stack' }] }
+          let(:buildpack_fields) do
+            [{ file: file, options: opts, stack: 'existing stack', config_index: 0 }, { file: another_file, options: opts, stack: 'manifest stack', config_index: 1 }]
+          end
 
           context 'and there are no matching Buildpacks' do
             it 'plans to create all the Buildpacks' do
@@ -277,7 +291,10 @@ module Jobs::Runtime
 
             let(:another_existing_stack) { Stack.make(name: 'another existing stack') }
             let!(:another_existing_buildpack) { Buildpack.make(name: name, stack: another_existing_stack.name, key: 'a_different_key', guid: 'a-different-guid') }
-            let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name }, { file: another_file, options: opts, stack: another_existing_stack.name }] }
+            let(:buildpack_fields) do
+              [{ file: file, options: opts, stack: existing_stack.name, config_index: 0 },
+               { file: another_file, options: opts, stack: another_existing_stack.name, config_index: 1 }]
+            end
 
             context 'and none of them has a nil stack' do
               it 'creates a job for each buildpack' do
@@ -297,7 +314,7 @@ module Jobs::Runtime
           end
 
           context 'when one of them is stackless' do
-            let(:buildpack_fields) { [{ file: file, options: opts }] }
+            let(:buildpack_fields) { [{ file: file, options: opts, config_index: 0 }] }
 
             before do
               Stack.make(name: 'existing stack')
@@ -316,7 +333,7 @@ module Jobs::Runtime
 
         context 'when the manifest has multiple buildpack entries for one name, with the same stack' do
           let(:another_file) { 'the-other-file' }
-          let(:buildpack_fields) { [{ file: file, options: opts, stack: 'stack' }, { file: another_file, options: opts, stack: 'stack' }] }
+          let(:buildpack_fields) { [{ file: file, options: opts, stack: 'stack', config_index: 0 }, { file: another_file, options: opts, stack: 'stack', config_index: 1 }] }
 
           it 'raises a DuplicateInstall error' do
             expect do
@@ -327,7 +344,7 @@ module Jobs::Runtime
 
         context 'when the manifest has multiple buildpack entries for one name, neither specifying a stack' do
           let(:another_file) { 'the-other-file' }
-          let(:buildpack_fields) { [{ file: file, options: opts, stack: nil }, { file: another_file, options: opts, stack: nil }] }
+          let(:buildpack_fields) { [{ file: file, options: opts, stack: nil, config_index: 0 }, { file: another_file, options: opts, stack: nil, config_index: 1 }] }
 
           it 'raises a DuplicateInstall error' do
             expect do
@@ -338,7 +355,7 @@ module Jobs::Runtime
 
         context 'when the manifest has multiple buildpack entries for one name, one stackful and one stackless' do
           let(:another_file) { 'the-other-file' }
-          let(:buildpack_fields) { [{ file: file, options: opts, stack: 'stack' }, { file: another_file, options: opts, stack: nil }] }
+          let(:buildpack_fields) { [{ file: file, options: opts, stack: 'stack', config_index: 0 }, { file: another_file, options: opts, stack: nil, config_index: 1 }] }
 
           it 'raises a StacklessBuildpackIncompatibilityError error' do
             expect do

spec/unit/jobs/runtime/create_buildpack_installer_spec.rb

@@ -78,6 +78,44 @@ module Jobs::Runtime
           end
         end
 
+        context 'when config_index is provided' do
+          let!(:existing_stack) { Stack.make(name: stack_name) }
+          let!(:existing_buildpack1) { Buildpack.make(name: 'first_buildpack', stack: stack_name) }
+          let!(:existing_buildpack2) { Buildpack.make(name: 'second_buildpack', stack: stack_name) }
+          let!(:existing_buildpack3) { Buildpack.make(name: 'third_buildpack', stack: stack_name) }
+          let!(:existing_buildpack4) { Buildpack.make(name: 'fourth_buildpack', stack: stack_name) }
+          let!(:existing_buildpack5) { Buildpack.make(name: 'fifth_buildpack', stack: stack_name) }
+          let(:job_options) { { name: 'mybuildpack', stack: stack_name, file: zipfile, options: { enabled: true, locked: false }, config_index: 5 } }
+
+          it 'moves the buildpack to position config_index + 1' do
+            job.perform
+            buildpack = Buildpack.find(name: 'mybuildpack')
+            expect(buildpack.position).to eq(6)
+          end
+        end
+
+        context 'when config_index is provided but options includes an explicit position' do
+          let!(:existing_stack) { Stack.make(name: stack_name) }
+          let(:job_options) { { name: 'mybuildpack', stack: stack_name, file: zipfile, options: { enabled: true, locked: false, position: 7 }, config_index: 5 } }
+
+          it 'uses the explicit position from options instead of config_index' do
+            job.perform
+            buildpack = Buildpack.find(name: 'mybuildpack')
+            expect(buildpack.position).to eq(7)
+          end
+        end
+
+        context 'when config_index is nil' do
+          let!(:existing_stack) { Stack.make(name: stack_name) }
+          let(:job_options) { { name: 'mybuildpack', stack: stack_name, file: zipfile, options: { enabled: true, locked: false } } }
+
+          it 'does not call move_to and uses default list position' do
+            job.perform
+            buildpack = Buildpack.find(name: 'mybuildpack')
+            expect(buildpack.position).to eq(1)
+          end
+        end
+
         context 'when uploading the buildpack fails' do
           let!(:existing_stack) { Stack.make(name: stack_name) }