Skip to content

Install buildpacks based on order on config #4981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomkennedy513 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Install buildpacks based on order on config #4981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/jobs/runtime/buildpack_installer_factory.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 9 additions & 0 deletions app/jobs/runtime/create_buildpack_installer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}'"

Expand All @@ -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))
Expand Down
72 changes: 46 additions & 26 deletions lib/cloud_controller/install_buildpacks.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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] }
Expand Down
49 changes: 33 additions & 16 deletions spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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'

Expand All @@ -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
Expand All @@ -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'

Expand All @@ -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'

Expand All @@ -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
Expand All @@ -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'

Expand All @@ -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'

Expand All @@ -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'

Expand All @@ -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
Expand All @@ -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'

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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')
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
38 changes: 38 additions & 0 deletions spec/unit/jobs/runtime/create_buildpack_installer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down
Loading
Loading