Skip to content
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.

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

Dt e3 0 update #304

Open
wants to merge 105 commits into
base: master
Choose a base branch
from
Open

Dt e3 0 update #304

wants to merge 105 commits into from

Conversation

SebastianKarpetaDev
Copy link

No description provided.

svenfuchs and others added 30 commits March 13, 2019 17:41
Added preferences method implementation to User and Organization classes.
When we migrate a build history it's possible that some jobs are still
running on .org. We shouldn't count them towards concurrency limits
because they're technically running on .org
When we migrate build history some of the builds may be in the created
state. We don't want to enqueue them as then they would be running on
both platforms.
ws_config is a Hashr, which doesn't have `#key?`
…190)

I've already attempted it to fix the problem in 01139a4, but I only
fixed one code path, which is currently not used, so the changes didn't
go into effect. This commit applies the changes to the other code path
as well.

The reason we need to do it is because when we migrate jobs from .org to
.com they might still be in a state that is considered by gatekeeper. So
they might be started but they're running on .org not .com, thus we
shouldn't count them as running on .com. The same with new jobs - we
don't want to start them as they would start on .org anyway.

I decided to filter these jobs in code instead of in the database
because the number of jobs that we fetch from the database is relatively
small and if I had to do it in the DB I would have to always join jobs
with repositories, which actually might be slower.
cherry pick commits from the public repo
This reverts commit e8f725d.
Allow ARM builds for Open Source only
Allow IBM power builds for Open Source only
Allow IBM z builds for Open Source only
Revert "Allow IBM z builds for Open Source only"
interval: 2,
limit: { public: 5, education: 1, default: 5, by_owner: {}, delegate: {} },
limit: { public: 93939, education: 92929, default: 91919, by_owner: {}, delegate: {} },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leave at 5,1,5 , the 93939 were tcie debug only

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -17,6 +17,10 @@ def accept?(job)
super if !on_metered_plan? || billing_allowed?(job)
end

def accept?(job)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double accept?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

config = record&.config_json if record.respond_to?(:config_json) # TODO: remove once we've rolled over
config ||= record&.config
config ||= read_attribute(:config) if has_attribute?(:config)
config = JSON.parse(config) if config.is_a?(String)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already at 114

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

end
return false if plan[:error] || plan['plan_name'].nil?
return false if plan[:error] || plan["plan_name"].nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep '

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


plan['hybrid'] || !plan['plan_name'].include?('free')
plan["hybrid"] || !plan["plan_name"].include?('free')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -117,7 +117,7 @@ def repository_data
default_branch: repo.default_branch,
description: repo.description,
server_type: repo.server_type || 'git'
)
server_type: repo.server_type || 'git',)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double setting and ',' (see 119)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

def vm_configs
config[:vm_configs] || {}
end
def vm_configs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indents broken

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -1066,4 +1066,155 @@ def subscribe(plan, owner = user)
end
end
end

context 'when user is on a metered plan' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if something related to billing is on enterprise branch and isn't on master - it was removed for some reason, better don't bring it back there

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

vm_config: {},
vm_type: :default,
vm_size: nil,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double vm_size

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -5,7 +5,8 @@
let(:repo) { FactoryBot.create(:repo) }
let(:owner) { FactoryBot.create(:user) }
let(:build) { FactoryBot.create(:build, repository: repo, owner:, jobs: [job]) }
let(:job) { FactoryBot.create(:job, private: true, state: :created, config: config.to_h) }
let(:job_stage) { FactoryGirl.create(:stage) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FactoryBot, there's no FactoryGirl anymore

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -5,6 +5,8 @@
let(:repo) { FactoryBot.create(:repo, owner:) }
let(:owner) { FactoryBot.create(:user) }
let(:commit) { FactoryBot.create(:commit) }
let(:build) { FactoryGirl.create(:build, repository: repo, owner: owner, jobs: [job]) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FactoryBot, there's no FactoryGirl anymore

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -28,7 +29,7 @@

it { expect(log).to include 'Evaluating jobs for owner group: user svenfuchs, org travis-ci' }
it { expect(log).to include "enqueueing job #{Job.first.id} (svenfuchs/gem-release)" }
it { expect(log).to include 'user svenfuchs, org travis-ci capacities: public max=5, config max=1' }
# it { expect(log).to include 'user svenfuchs, org travis-ci capacities: public max=5, config max=1' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was debug only for tcie, can be enabled

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -45,4 +46,24 @@
expect(log).to include "I 1234 Owner group scheduler.owners-svenfuchs is locked and already being evaluated. Dropping event build:created for build=#{build.id}"
}
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason removed on master, don't add it (also factorygirl used)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -1,7 +1,9 @@
# frozen_string_literal: false

describe Travis::Scheduler::Service::Notify do
let(:job) { FactoryBot.create(:job, state: :queued, queued_at: Time.parse('2016-01-01T10:30:00Z'), config: {}) }
let(:build) { FactoryGirl.create(:build, repository: repo, owner: owner, jobs: [job]) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factorygirl -> factorybot

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factorygirl is still here for 'build'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@gbarc-dt gbarc-dt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found some factorygirl calls - pls grep the sources and replace all with factorybot

@@ -1,7 +1,9 @@
# frozen_string_literal: false

describe Travis::Scheduler::Service::Notify do
let(:job) { FactoryBot.create(:job, state: :queued, queued_at: Time.parse('2016-01-01T10:30:00Z'), config: {}) }
let(:build) { FactoryGirl.create(:build, repository: repo, owner: owner, jobs: [job]) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factorygirl is still here for 'build'

@@ -5,6 +5,8 @@
let(:repo) { FactoryBot.create(:repo, owner:) }
let(:owner) { FactoryBot.create(:user) }
let(:commit) { FactoryBot.create(:commit) }
let(:build) { FactoryBot.create(:build, repository: repo, owner: owner, jobs: [job]) }
let(:job_stage) { FactoryGirl.create(:stage) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factorygirl for 'job_stage' - change to factorybot

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

end

context 'when head repo is present' do
let(:head_repo) { FactoryGirl.create(:repository, github_id: 549744) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factorygirl - change to factorybot

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -4,6 +4,8 @@
let(:user) { FactoryBot.create(:user) }
let(:repo) { FactoryBot.create(:repository) }
let(:authorize_build_url) { "http://localhost:9292/users/#{user.id}/plan" }
let(:repo) { FactoryGirl.create(:repository) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factorygirl - change to factorybot

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

rescue => e
puts "ERROR while trying to queue: #{e.message}"
puts "Backtrace:"
puts e.backtrace.join("\n")@queue ||= redirect(Queue.new(job, config, logger).select)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline missing before @Queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.