From ec326c805e142eb4643b14250fb57ecb8c14b5eb Mon Sep 17 00:00:00 2001 From: Jey Balachandran Date: Sun, 25 Oct 2015 14:25:51 -0700 Subject: [PATCH 1/2] Create Environment model for better organization. Deployment now also stores the sender's username and avatar url which can be used in the future for views. In addition, some indexes were added for optimization. --- app/models/deployment.rb | 9 ++--- app/models/environment.rb | 5 +++ ...20150903184055_add_repositories_indexes.rb | 5 +++ .../20150903204319_create_environments.rb | 33 +++++++++++++++++++ ...04161019_add_avatar_url_for_deployments.rb | 9 +++++ db/schema.rb | 20 +++++++++-- lib/heaven/provider/default_provider.rb | 26 ++++++++++----- spec/models/deployment_spec.rb | 9 +++-- spec/models/environment_spec.rb | 10 ++++++ 9 files changed, 110 insertions(+), 16 deletions(-) create mode 100644 app/models/environment.rb create mode 100644 db/migrate/20150903184055_add_repositories_indexes.rb create mode 100644 db/migrate/20150903204319_create_environments.rb create mode 100644 db/migrate/20150904161019_add_avatar_url_for_deployments.rb create mode 100644 spec/models/environment_spec.rb diff --git a/app/models/deployment.rb b/app/models/deployment.rb index d14a29e60..dfbe5c143 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -1,18 +1,19 @@ # A record of a deployment processes class Deployment < ActiveRecord::Base - validates_presence_of :name, :name_with_owner + validates :name, :name_with_owner, :environment, :repository, presence: true + belongs_to :environment belongs_to :repository def self.latest_for_name_with_owner(name_with_owner) - sets = self.select(:name, :environment) + sets = self.select(:name, :environment_id) .where(:name_with_owner => name_with_owner) - .group("name,environment") + .group([:name, :environment_id]) sets.map do |deployment| params = { :name => deployment.name, - :environment => deployment.environment, + :environment_id => deployment.environment_id, :name_with_owner => name_with_owner } Deployment.where(params).order("created_at desc").limit(1) diff --git a/app/models/environment.rb b/app/models/environment.rb new file mode 100644 index 000000000..87d47c0a4 --- /dev/null +++ b/app/models/environment.rb @@ -0,0 +1,5 @@ +class Environment < ActiveRecord::Base + validates :name, presence: true, uniqueness: true + + has_many :deployments, dependent: :delete_all +end diff --git a/db/migrate/20150903184055_add_repositories_indexes.rb b/db/migrate/20150903184055_add_repositories_indexes.rb new file mode 100644 index 000000000..325b7a431 --- /dev/null +++ b/db/migrate/20150903184055_add_repositories_indexes.rb @@ -0,0 +1,5 @@ +class AddRepositoriesIndexes < ActiveRecord::Migration + def change + add_index :repositories, [:owner, :name], unique: true + end +end diff --git a/db/migrate/20150903204319_create_environments.rb b/db/migrate/20150903204319_create_environments.rb new file mode 100644 index 000000000..b07af1a56 --- /dev/null +++ b/db/migrate/20150903204319_create_environments.rb @@ -0,0 +1,33 @@ +class CreateEnvironments < ActiveRecord::Migration + def up + create_table :environments do |t| + t.string :name, required: true, index: true, unique: true + t.timestamps + end + + add_column :deployments, :environment_id, :integer + + Deployment.connection.schema_cache.clear! + Deployment.reset_column_information + Deployment.find_each do |deployment| + deployment.update_column(:environment_id, Environment.where(name: deployment.read_attribute(:environment)).first_or_create!.id) + end + + change_column :deployments, :environment_id, :integer, null: false + remove_column :deployments, :environment + end + + def down + add_column :deployments, :environment, :string, default: "production" + + Deployment.connection.schema_cache.clear! + Deployment.reset_column_information + Deployment.includes(:environment).find_each do |deployment| + deployment.update_column(:environment, Environment.find(deployment.environment_id).first!.name) + end + + remove_column :deployments, :environment_id + + drop_table :environments + end +end diff --git a/db/migrate/20150904161019_add_avatar_url_for_deployments.rb b/db/migrate/20150904161019_add_avatar_url_for_deployments.rb new file mode 100644 index 000000000..ee1d47d97 --- /dev/null +++ b/db/migrate/20150904161019_add_avatar_url_for_deployments.rb @@ -0,0 +1,9 @@ +class AddAvatarUrlForDeployments < ActiveRecord::Migration + def change + add_column :deployments, :sender_login, :string + add_column :deployments, :sender_avatar_url, :string + add_index :deployments, [:repository_id, :environment_id] + add_index :deployments, [:name, :environment_id, :name_with_owner], name: "index_deployments_on_latest_for_name_with_owner" + add_index :deployments, :created_at + end +end diff --git a/db/schema.rb b/db/schema.rb index 67b265146..441ce59d3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,11 +11,10 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20140728040201) do +ActiveRecord::Schema.define(version: 20150904161019) do create_table "deployments", force: :cascade do |t| t.text "custom_payload" - t.string "environment", default: "production" t.string "guid" t.string "name" t.string "name_with_owner" @@ -25,8 +24,23 @@ t.datetime "created_at" t.datetime "updated_at" t.integer "repository_id" + t.integer "environment_id", null: false + t.string "sender" + t.string "avatar_url" end + add_index "deployments", ["created_at"], name: "index_deployments_on_created_at" + add_index "deployments", ["name", "environment_id", "name_with_owner"], name: "index_deployments_on_latest_for_name_with_owner" + add_index "deployments", ["repository_id", "environment_id"], name: "index_deployments_on_repository_id_and_environment_id" + + create_table "environments", force: :cascade do |t| + t.string "name" + t.datetime "created_at" + t.datetime "updated_at" + end + + add_index "environments", ["name"], name: "index_environments_on_name" + create_table "repositories", force: :cascade do |t| t.string "owner" t.string "name" @@ -35,4 +49,6 @@ t.datetime "updated_at" end + add_index "repositories", ["owner", "name"], name: "index_repositories_on_owner_and_name", unique: true + end diff --git a/lib/heaven/provider/default_provider.rb b/lib/heaven/provider/default_provider.rb index 38f719e80..03c93e4de 100644 --- a/lib/heaven/provider/default_provider.rb +++ b/lib/heaven/provider/default_provider.rb @@ -61,6 +61,14 @@ def name_without_owner data["repository"]["name"] end + def sender_login + data["sender"]["login"] + end + + def sender_avatar_url + data["sender"]["avatar_url"] + end + def sha deployment_data["sha"][0..7] end @@ -137,14 +145,16 @@ def notify end def record - Deployment.create(:custom_payload => JSON.dump(custom_payload), - :environment => environment, - :guid => guid, - :name => name, - :name_with_owner => name_with_owner, - :output => output.url, - :ref => ref, - :sha => sha) + Deployment.create(:custom_payload => JSON.dump(custom_payload), + :environment => environment, + :guid => guid, + :name => name, + :name_with_owner => name_with_owner, + :output => output.url, + :ref => ref, + :sha => sha, + :sender_login => sender_login, + :sender_avatar_url => sender_avatar_url) end def update_output diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index c2b720c9e..b52ac7490 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -6,10 +6,15 @@ let(:payload) { fixture_data("deployment") } let!(:data) { JSON.parse(payload)["payload"] } + let(:repository) { Repository.where(owner: "atmos", name: "heaven").first_or_create! } + let(:production) { Environment.where(name: "production").first_or_create! } + let(:staging) { Environment.where(name: "staging").first_or_create! } + let!(:create_data) do { :custom_payload => JSON.dump(data), - :environment => "production", + :repository => repository, + :environment => production, :guid => SecureRandom.uuid, :name => "hubot", :name_with_owner => "github/hubot", @@ -34,7 +39,7 @@ Deployment.create create_data.merge(:name_with_owner => "atmos/heaven") - present << Deployment.create(create_data.merge(:environment => "staging")) + present << Deployment.create(create_data.merge(:environment => staging)) deployments = Deployment.latest_for_name_with_owner("github/hubot") diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb new file mode 100644 index 000000000..0aa2259ff --- /dev/null +++ b/spec/models/environment_spec.rb @@ -0,0 +1,10 @@ +require "spec_helper" + +describe Environment do + it "doesn't recreate environments with the same name" do + expect(Environment.create(name: "production")).to be_valid + expect { + Environment.create!(name: "production") + }.to raise_exception + end +end From 8a7d7b902153c2c53e4933c535d3953b14baab66 Mon Sep 17 00:00:00 2001 From: Jey Balachandran Date: Sun, 25 Oct 2015 14:49:16 -0700 Subject: [PATCH 2/2] Appease RuboCop. --- app/models/deployment.rb | 5 +++-- app/models/environment.rb | 5 +++-- db/schema.rb | 6 +++--- lib/heaven/provider/default_provider.rb | 12 ++++-------- spec/models/deployment_spec.rb | 6 +++--- spec/models/environment_spec.rb | 6 ++---- 6 files changed, 18 insertions(+), 22 deletions(-) diff --git a/app/models/deployment.rb b/app/models/deployment.rb index dfbe5c143..acec1d24d 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -1,6 +1,7 @@ # A record of a deployment processes class Deployment < ActiveRecord::Base - validates :name, :name_with_owner, :environment, :repository, presence: true + validates :name, :name_with_owner, :environment, :repository, + :presence => true belongs_to :environment belongs_to :repository @@ -16,7 +17,7 @@ def self.latest_for_name_with_owner(name_with_owner) :environment_id => deployment.environment_id, :name_with_owner => name_with_owner } - Deployment.where(params).order("created_at desc").limit(1) + Deployment.where(params).order(arel_table[:created_at].desc).limit(1) end.flatten end diff --git a/app/models/environment.rb b/app/models/environment.rb index 87d47c0a4..f1624e3ea 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -1,5 +1,6 @@ +# A location for the deployment (i.e. production, staging) class Environment < ActiveRecord::Base - validates :name, presence: true, uniqueness: true + validates :name, :presence => true, :uniqueness => true - has_many :deployments, dependent: :delete_all + has_many :deployments, :dependent => :delete_all end diff --git a/db/schema.rb b/db/schema.rb index 441ce59d3..8b89a8e76 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -24,9 +24,9 @@ t.datetime "created_at" t.datetime "updated_at" t.integer "repository_id" - t.integer "environment_id", null: false - t.string "sender" - t.string "avatar_url" + t.integer "environment_id", null: false + t.string "sender_login" + t.string "sender_avatar_url" end add_index "deployments", ["created_at"], name: "index_deployments_on_created_at" diff --git a/lib/heaven/provider/default_provider.rb b/lib/heaven/provider/default_provider.rb index 03c93e4de..8287c2374 100644 --- a/lib/heaven/provider/default_provider.rb +++ b/lib/heaven/provider/default_provider.rb @@ -61,12 +61,8 @@ def name_without_owner data["repository"]["name"] end - def sender_login - data["sender"]["login"] - end - - def sender_avatar_url - data["sender"]["avatar_url"] + def sender + data["sender"] end def sha @@ -153,8 +149,8 @@ def record :output => output.url, :ref => ref, :sha => sha, - :sender_login => sender_login, - :sender_avatar_url => sender_avatar_url) + :sender_login => sender["login"], + :sender_avatar_url => sender["avatar_url"]) end def update_output diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index b52ac7490..23caea680 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -6,9 +6,9 @@ let(:payload) { fixture_data("deployment") } let!(:data) { JSON.parse(payload)["payload"] } - let(:repository) { Repository.where(owner: "atmos", name: "heaven").first_or_create! } - let(:production) { Environment.where(name: "production").first_or_create! } - let(:staging) { Environment.where(name: "staging").first_or_create! } + let(:repository) { Repository.create(:owner => "atmos", :name => "heaven") } + let(:production) { Environment.create(:name => "production") } + let(:staging) { Environment.create(:name => "staging") } let!(:create_data) do { diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 0aa2259ff..80f365ae1 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -2,9 +2,7 @@ describe Environment do it "doesn't recreate environments with the same name" do - expect(Environment.create(name: "production")).to be_valid - expect { - Environment.create!(name: "production") - }.to raise_exception + expect(Environment.create(:name => "production")).to be_valid + expect { Environment.create!(:name => "production") }.to raise_exception end end