From eae37afe9c3c252887bf1e40fe4b94438dbc40f6 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 5 Dec 2018 11:49:50 -0800 Subject: [PATCH 1/3] Typos and whitespace fixes --- lib/travis/scheduler/limit/jobs.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/travis/scheduler/limit/jobs.rb b/lib/travis/scheduler/limit/jobs.rb index b3fdaa3d..d747284b 100644 --- a/lib/travis/scheduler/limit/jobs.rb +++ b/lib/travis/scheduler/limit/jobs.rb @@ -66,7 +66,7 @@ def check_all queueable.each_with_index do |job, index| case check(job) when :limited - # The rest of the jobs will definitely be waiting for + # The rest of the jobs will definitely be waiting for # concurrency, regardless of other limits that might apply @waiting_by_owner += queueable.length - index break From 910008de8b52a9a2783c4d7e1b905ffdcd841870 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 5 Dec 2018 13:44:46 -0800 Subject: [PATCH 2/3] Increase the job capacity of all plans by 1 This work is part of the "1 free private repo job" plan. https://github.com/travis-ci/product/issues/97 --- lib/travis/owners/subscriptions.rb | 21 ++++++++++- spec/travis/owners/subscriptions_spec.rb | 7 ++++ spec/travis/owners_spec.rb | 4 +- spec/travis/scheduler/jobs_spec.rb | 48 ++++++++++++------------ spec/travis/scheduler/limit/com_spec.rb | 34 ++++++++--------- 5 files changed, 69 insertions(+), 45 deletions(-) diff --git a/lib/travis/owners/subscriptions.rb b/lib/travis/owners/subscriptions.rb index 1a4081b1..e1c4d6f0 100644 --- a/lib/travis/owners/subscriptions.rb +++ b/lib/travis/owners/subscriptions.rb @@ -25,7 +25,20 @@ def plan_limits end def plan_limit(plan) - config[plan.to_sym].tap { |limit| missing_plan(plan) unless limit } + limit = config[plan[:plan].to_sym].tap { |limit| missing_plan(plan[:plan]) unless limit } + + # Increment this by 1 as our original values are being increased up by + # one as part of our "free 1-job private repo" plan project, + # expected to be launched in December, 2018. + # + # https://github.com/travis-ci/product/issues/97 + # + + if plan[:owner].is_a?(User) && !limit.nil? + limit += 1 unless limit.nil? + end + + limit end def missing_plan(plan) @@ -33,7 +46,11 @@ def missing_plan(plan) end def plans - subscriptions.map(&:selected_plan).compact + subscriptions.map { |sub| + if sub.selected_plan && sub.owner + { plan: sub.selected_plan, owner: sub.owner } + end + }.compact end def subscriptions diff --git a/spec/travis/owners/subscriptions_spec.rb b/spec/travis/owners/subscriptions_spec.rb index e4dede3d..9300b299 100644 --- a/spec/travis/owners/subscriptions_spec.rb +++ b/spec/travis/owners/subscriptions_spec.rb @@ -10,6 +10,13 @@ subject { described_class.new(owners, plans).max_jobs } + # Note that all plans are 1 higher than their original values due to our "free + # 1-job private repo" plan project, expected to be launched in December, + # 2018. + # + # https://github.com/travis-ci/product/issues/97 + # + describe 'a single org with a five jobs plan' do before { FactoryGirl.create(:subscription, owner: travis, selected_plan: :five) } it { should eq 5 } diff --git a/spec/travis/owners_spec.rb b/spec/travis/owners_spec.rb index c9de3b0e..bbbc0a2a 100644 --- a/spec/travis/owners_spec.rb +++ b/spec/travis/owners_spec.rb @@ -16,7 +16,7 @@ describe 'with a subscription on the delegatee' do before { FactoryGirl.create(:subscription, owner: anja, selected_plan: :ten) } - it { expect(owners.max_jobs).to eq 10 } + it { expect(owners.max_jobs).to eq 11 } end describe 'with a subscription on the delegate' do @@ -27,7 +27,7 @@ describe 'with a subscription on both the delegatee and delegate' do before { FactoryGirl.create(:subscription, owner: anja, selected_plan: :ten) } before { FactoryGirl.create(:subscription, owner: travis, selected_plan: :five) } - it { expect(owners.max_jobs).to eq 15 } + it { expect(owners.max_jobs).to eq 16 } end end diff --git a/spec/travis/scheduler/jobs_spec.rb b/spec/travis/scheduler/jobs_spec.rb index ba00aaec..55c8a0a9 100644 --- a/spec/travis/scheduler/jobs_spec.rb +++ b/spec/travis/scheduler/jobs_spec.rb @@ -86,21 +86,21 @@ def subscribe(plan, owner = self.user) before { create_jobs(1, private: true, state: :started) } before { create_jobs(5, private: true) } - it { expect(selected.size).to eq 1 } - it { expect(reports).to include 'user svenfuchs capacities: public max=3, plan max=2' } - it { expect(reports).to include 'user svenfuchs plan capacity: running=1 max=2 selected=1' } - it { expect(reports).to include 'user svenfuchs: queueable=5 running=1 selected=1 total_waiting=4 waiting_for_concurrency=4' } + it { expect(selected.size).to eq 2 } + it { expect(reports).to include 'user svenfuchs capacities: public max=3, plan max=3' } + it { expect(reports).to include 'user svenfuchs plan capacity: running=1 max=3 selected=2' } + it { expect(reports).to include 'user svenfuchs: queueable=5 running=1 selected=2 total_waiting=3 waiting_for_concurrency=3' } end describe 'with public jobs only' do before { create_jobs(1, private: false, state: :started) } before { create_jobs(5, private: false) } - it { expect(selected.size).to eq 4 } - it { expect(reports).to include 'user svenfuchs capacities: public max=3, plan max=2' } + it { expect(selected.size).to eq 5 } + it { expect(reports).to include 'user svenfuchs capacities: public max=3, plan max=3' } it { expect(reports).to include 'user svenfuchs public capacity: running=1 max=3 selected=2' } - it { expect(reports).to include 'user svenfuchs plan capacity: running=0 max=2 selected=2' } - it { expect(reports).to include 'user svenfuchs: queueable=5 running=1 selected=4 total_waiting=1 waiting_for_concurrency=1' } + it { expect(reports).to include 'user svenfuchs plan capacity: running=0 max=3 selected=3' } + it { expect(reports).to include 'user svenfuchs: queueable=5 running=1 selected=5 total_waiting=0 waiting_for_concurrency=0' } end describe 'for mixed public and private jobs' do @@ -108,11 +108,11 @@ def subscribe(plan, owner = self.user) before { create_jobs(1, private: false, state: :started) } before { create_jobs(2, private: false) + create_jobs(2, private: true) } - it { expect(selected.size).to eq 3 } - it { expect(reports).to include 'user svenfuchs capacities: public max=3, plan max=2' } + it { expect(selected.size).to eq 4 } + it { expect(reports).to include 'user svenfuchs capacities: public max=3, plan max=3' } it { expect(reports).to include 'user svenfuchs public capacity: running=1 max=3 selected=2' } - it { expect(reports).to include 'user svenfuchs plan capacity: running=1 max=2 selected=1' } - it { expect(reports).to include 'user svenfuchs: queueable=4 running=2 selected=3 total_waiting=1 waiting_for_concurrency=1' } + it { expect(reports).to include 'user svenfuchs plan capacity: running=1 max=3 selected=2' } + it { expect(reports).to include 'user svenfuchs: queueable=4 running=2 selected=4 total_waiting=0 waiting_for_concurrency=0' } end end @@ -232,19 +232,19 @@ def subscribe(plan, owner = self.user) before { create_jobs(1, private: true, state: :started) } before { create_jobs(5, private: true) } - it { expect(selected.size).to eq 1 } - it { expect(reports).to include 'user svenfuchs plan capacity: running=1 max=2 selected=1' } - it { expect(reports).to include 'user svenfuchs: queueable=5 running=1 selected=1 total_waiting=4 waiting_for_concurrency=4' } + it { expect(selected.size).to eq 2 } + it { expect(reports).to include 'user svenfuchs plan capacity: running=1 max=3 selected=2' } + it { expect(reports).to include 'user svenfuchs: queueable=5 running=1 selected=2 total_waiting=3 waiting_for_concurrency=3' } end describe 'with public jobs only' do before { create_jobs(1, private: false, state: :started) } before { create_jobs(5, private: false) } - it { expect(selected.size).to eq 4 } + it { expect(selected.size).to eq 5 } it { expect(reports).to include 'user svenfuchs public capacity: running=1 max=3 selected=2' } - it { expect(reports).to include 'user svenfuchs plan capacity: running=0 max=2 selected=2' } - it { expect(reports).to include 'user svenfuchs: queueable=5 running=1 selected=4 total_waiting=1 waiting_for_concurrency=1' } + it { expect(reports).to include 'user svenfuchs plan capacity: running=0 max=3 selected=3' } + it { expect(reports).to include 'user svenfuchs: queueable=5 running=1 selected=5 total_waiting=0 waiting_for_concurrency=0' } end describe 'for mixed public and private jobs' do @@ -252,10 +252,10 @@ def subscribe(plan, owner = self.user) before { create_jobs(1, private: false, state: :started) } before { create_jobs(2, private: false) + create_jobs(2, private: true) } - it { expect(selected.size).to eq 3 } + it { expect(selected.size).to eq 4 } it { expect(reports).to include 'user svenfuchs public capacity: running=1 max=3 selected=2' } - it { expect(reports).to include 'user svenfuchs plan capacity: running=1 max=2 selected=1' } - it { expect(reports).to include 'user svenfuchs: queueable=4 running=2 selected=3 total_waiting=1 waiting_for_concurrency=1' } + it { expect(reports).to include 'user svenfuchs plan capacity: running=1 max=3 selected=2' } + it { expect(reports).to include 'user svenfuchs: queueable=4 running=2 selected=4 total_waiting=0 waiting_for_concurrency=0' } end end @@ -381,7 +381,7 @@ def subscribe(plan, owner = self.user) it { expect(selected.size).to eq 3 } it { expect(reports).to include 'user svenfuchs limited by queue builds.osx: max=3 rejected=7 selected=2' } - it { expect(reports).to include 'user svenfuchs plan capacity: running=2 max=10 selected=3' } + it { expect(reports).to include 'user svenfuchs plan capacity: running=2 max=11 selected=3' } it { expect(reports).to include 'user svenfuchs: queueable=10 running=2 selected=3 total_waiting=7 waiting_for_concurrency=0' } end @@ -394,7 +394,7 @@ def subscribe(plan, owner = self.user) it { expect(selected.size).to eq 3 } it { expect(reports).to include 'user svenfuchs limited by queue builds.osx: max=3 rejected=7 selected=2' } it { expect(reports).to include 'user svenfuchs public capacity: running=2 max=3 selected=1' } - it { expect(reports).to include 'user svenfuchs plan capacity: running=0 max=10 selected=2' } + it { expect(reports).to include 'user svenfuchs plan capacity: running=0 max=11 selected=2' } it { expect(reports).to include 'user svenfuchs: queueable=10 running=2 selected=3 total_waiting=7 waiting_for_concurrency=0' } end @@ -409,7 +409,7 @@ def subscribe(plan, owner = self.user) it { expect(selected.size).to eq 4 } it { expect(reports).to include 'user svenfuchs limited by queue builds.osx: max=3 rejected=6 selected=2' } it { expect(reports).to include 'user svenfuchs public capacity: running=0 max=3 selected=1' } - it { expect(reports).to include 'user svenfuchs plan capacity: running=2 max=10 selected=3' } + it { expect(reports).to include 'user svenfuchs plan capacity: running=2 max=11 selected=3' } it { expect(reports).to include 'user svenfuchs: queueable=10 running=2 selected=4 total_waiting=6 waiting_for_concurrency=0' } end end diff --git a/spec/travis/scheduler/limit/com_spec.rb b/spec/travis/scheduler/limit/com_spec.rb index 57fd361a..01c98d9e 100644 --- a/spec/travis/scheduler/limit/com_spec.rb +++ b/spec/travis/scheduler/limit/com_spec.rb @@ -84,10 +84,10 @@ def subscription(plan, owner = self.owner) before { create_jobs(2, state: :created, private: true) } before { run } - it { expect(selected).to eq 1 } - it { expect(report).to include('max jobs for user svenfuchs by plan: 2 (svenfuchs)') } - it { expect(report).to include('user svenfuchs: total: 2, running: 1, queueable: 1') } - it { expect(limit.waiting_by_owner).to eq 1 } + it { expect(selected).to eq 2 } + it { expect(report).to include('max jobs for user svenfuchs by plan: 3 (svenfuchs)') } + it { expect(report).to include('user svenfuchs: total: 2, running: 1, queueable: 2') } + it { expect(limit.waiting_by_owner).to eq 0 } end describe 'with public jobs only' do @@ -97,7 +97,7 @@ def subscription(plan, owner = self.owner) before { run } it { expect(selected).to eq 2 } - it { expect(report).to include('max jobs for user svenfuchs by plan: 5 (svenfuchs)') } # TODO fix log output? + it { expect(report).to include('max jobs for user svenfuchs by plan: 6 (svenfuchs)') } # TODO fix log output? it { expect(report).to include('user svenfuchs: total: 2, running: 1, queueable: 2') } it { expect(limit.waiting_by_owner).to eq 0 } end @@ -110,10 +110,10 @@ def subscription(plan, owner = self.owner) before { subscription(:two) } before { run } - it { expect(selected).to eq 3 } - it { expect(report).to include('max jobs for user svenfuchs by plan: 2 (svenfuchs)') } - it { expect(report).to include('user svenfuchs: total: 4, running: 2, queueable: 3') } - it { expect(limit.waiting_by_owner).to eq 1 } + it { expect(selected).to eq 4 } + it { expect(report).to include('max jobs for user svenfuchs by plan: 3 (svenfuchs)') } + it { expect(report).to include('user svenfuchs: total: 4, running: 2, queueable: 4') } + it { expect(limit.waiting_by_owner).to eq 0 } end end @@ -303,7 +303,7 @@ def subscription(plan, owner = self.owner) before { run } it { expect(selected).to eq 2 } - it { expect(report).to include('max jobs for user svenfuchs by plan: 4 (svenfuchs)') } + it { expect(report).to include('max jobs for user svenfuchs by plan: 5 (svenfuchs)') } it { expect(report).to include('max jobs for repo svenfuchs/gem-release by repo_settings: 3') } it { expect(report).to include('user svenfuchs: total: 4, running: 1, queueable: 2') } it { expect(limit.waiting_by_owner).to eq 0 } # TODO eh??? should be 2, no? @@ -315,7 +315,7 @@ def subscription(plan, owner = self.owner) before { run } it { expect(selected).to eq 2 } - it { expect(report).to include('max jobs for user svenfuchs by plan: 7 (svenfuchs)') } # TODO fix log output? + it { expect(report).to include('max jobs for user svenfuchs by plan: 8 (svenfuchs)') } # TODO fix log output? it { expect(report).to include('max jobs for repo svenfuchs/gem-release by repo_settings: 3') } it { expect(report).to include('user svenfuchs: total: 4, running: 1, queueable: 2') } it { expect(limit.waiting_by_owner).to eq 0 } @@ -329,7 +329,7 @@ def subscription(plan, owner = self.owner) before { run } it { expect(selected).to eq 1 } - it { expect(report).to include('max jobs for user svenfuchs by plan: 4 (svenfuchs)') } + it { expect(report).to include('max jobs for user svenfuchs by plan: 5 (svenfuchs)') } it { expect(report).to include('max jobs for repo svenfuchs/gem-release by repo_settings: 3') } it { expect(report).to include('user svenfuchs: total: 6, running: 2, queueable: 1') } it { expect(limit.waiting_by_owner).to eq 0 } # TODO eh??? @@ -343,8 +343,8 @@ def subscription(plan, owner = self.owner) before { subscription(:two) } before { run } - it { expect(selected).to eq 2 } - it { expect(report).to include('user svenfuchs: total: 4, running: 0, queueable: 2') } + it { expect(selected).to eq 3 } + it { expect(report).to include('user svenfuchs: total: 4, running: 0, queueable: 3') } end describe 'with public jobs only' do @@ -397,7 +397,7 @@ def subscription(plan, owner = self.owner) it { expect(selected).to eq 6 } it { expect(limit.selected.map(&:owner).map(&:login)).to eq ['svenfuchs'] * 3 + ['travis-ci'] * 3 } - it { expect(report).to include('max jobs for user svenfuchs by plan: 8 (svenfuchs, travis-ci)') } + it { expect(report).to include('max jobs for user svenfuchs by plan: 9 (svenfuchs, travis-ci)') } it { expect(report).to include('user svenfuchs, user carla, org travis-ci: total: 6, running: 2, queueable: 6') } end end @@ -430,7 +430,7 @@ def subscription(plan, owner = self.owner) it { expect(selected).to eq 6 } it { expect(limit.selected.map(&:owner).map(&:login)).to eq ['svenfuchs'] * 3 + ['travis-ci'] * 3 } - it { expect(report).to include('max jobs for user svenfuchs by plan: 8 (svenfuchs, travis-ci)') } # TODO fix log output + it { expect(report).to include('max jobs for user svenfuchs by plan: 9 (svenfuchs, travis-ci)') } # TODO fix log output it { expect(report).to include('user svenfuchs, user carla, org travis-ci: total: 6, running: 2, queueable: 6') } end end @@ -463,7 +463,7 @@ def subscription(plan, owner = self.owner) it { expect(selected).to eq 6 } it { expect(limit.selected.map(&:owner).map(&:login)).to eq ['svenfuchs'] * 3 + ['travis-ci'] * 3 } - it { expect(report).to include('max jobs for user svenfuchs by plan: 8 (svenfuchs, travis-ci)') } + it { expect(report).to include('max jobs for user svenfuchs by plan: 9 (svenfuchs, travis-ci)') } it { expect(report).to include('user svenfuchs, user carla, org travis-ci: total: 6, running: 2, queueable: 6') } end end From 0b682ee71161b59b059be9f33f90c91ef45134b1 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Wed, 5 Dec 2018 15:50:22 -0800 Subject: [PATCH 3/3] Cleanup logic usage --- lib/travis/owners/subscriptions.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/travis/owners/subscriptions.rb b/lib/travis/owners/subscriptions.rb index e1c4d6f0..75393d30 100644 --- a/lib/travis/owners/subscriptions.rb +++ b/lib/travis/owners/subscriptions.rb @@ -34,8 +34,8 @@ def plan_limit(plan) # https://github.com/travis-ci/product/issues/97 # - if plan[:owner].is_a?(User) && !limit.nil? - limit += 1 unless limit.nil? + if plan[:owner].is_a?(User) && limit + limit += 1 end limit