Merge pull request #963 from powerhome/elevate-repository-as-first-cl…

…ass-domain-concept

Elevate repository as first class domain concept

app/controllers/shipit/api/stacks_controller.rb

@@ -18,7 +18,10 @@ def index
         accepts :merge_queue_enabled, Boolean
       end
       def create
-        render_resource Stack.create(params)
+        stack = Stack.new(create_params)
+        stack.repository = repository
+        stack.save
+        render_resource stack
       end
 
       def show
@@ -32,9 +35,25 @@ def destroy
 
       private
 
+      def create_params
+        params.reject { |key, _| %i(repo_owner repo_name).include?(key) }
+      end
+
       def stack
         @stack ||= stacks.from_param!(params[:id])
       end
+
+      def repository
+        @repository ||= Repository.find_or_create_by(owner: repo_owner, name: repo_name)
+      end
+
+      def repo_owner
+        params[:repo_owner]
+      end
+
+      def repo_name
+        params[:repo_name]
+      end
     end
   end
 end

app/controllers/shipit/merge_status_controller.rb

@@ -60,9 +60,11 @@ def stack
       @stack ||= if params[:stack_id]
         Stack.from_param!(params[:stack_id])
       else
-        scope = Stack.order(merge_queue_enabled: :desc, id: :asc).where(
-          repo_owner: referrer_parser.repo_owner,
-          repo_name: referrer_parser.repo_name,
+        scope = Stack.order(merge_queue_enabled: :desc, id: :asc).includes(:repository).where(
+          repositories: {
+            owner: referrer_parser.repo_owner,
+            name: referrer_parser.repo_name,
+          },
         )
         scope = if params[:branch]
           scope.where(branch: params[:branch])

app/controllers/shipit/stacks_controller.rb

@@ -48,6 +48,7 @@ def lookup
 
     def create
       @stack = Stack.new(create_params)
+      @stack.repository = repository
       unless @stack.save
         flash[:warning] = @stack.errors.full_messages.to_sentence
       end
@@ -112,7 +113,7 @@ def load_stack
     end
 
     def create_params
-      params.require(:stack).permit(:repo_name, :repo_owner, :environment, :branch, :deploy_url, :ignore_ci)
+      params.require(:stack).permit(:environment, :branch, :deploy_url, :ignore_ci)
     end
 
     def update_params
@@ -124,5 +125,21 @@ def update_params
         :merge_queue_enabled,
       )
     end
+
+    def repository
+      @repository ||= Repository.find_or_create_by(owner: repo_owner, name: repo_name)
+    end
+
+    def repo_owner
+      repository_params[:repo_owner]
+    end
+
+    def repo_name
+      repository_params[:repo_name]
+    end
+
+    def repository_params
+      params.require(:stack).permit(:repo_owner, :repo_name)
+    end
   end
 end

app/controllers/shipit/webhooks_controller.rb

@@ -28,7 +28,7 @@ def process
       private
 
       def stacks
-        @stacks ||= Stack.repo(repository_name)
+        @stacks ||= Repository.from_github_repo_name(repository_name).stacks
       end
 
       def repository_name

app/models/shipit/repository.rb

@@ -0,0 +1,38 @@
+module Shipit
+  class Repository < ApplicationRecord
+    OWNER_MAX_SIZE = 39
+    private_constant :OWNER_MAX_SIZE
+
+    NAME_MAX_SIZE = 100
+    private_constant :NAME_MAX_SIZE
+
+    validates :name, uniqueness: {scope: %i(owner), case_sensitive: false,
+                                  message: 'cannot be used more than once'}
+    validates :owner, :name, presence: true, ascii_only: true
+    validates :owner, format: {with: /\A[a-z0-9_\-\.]+\z/}, length: {maximum: OWNER_MAX_SIZE}
+    validates :name, format: {with: /\A[a-z0-9_\-\.]+\z/}, length: {maximum: NAME_MAX_SIZE}
+
+    has_many :stacks, dependent: :destroy
+
+    def self.from_github_repo_name(github_repo_name)
+      repo_owner, repo_name = github_repo_name.downcase.split('/')
+      find_by(owner: repo_owner, name: repo_name)
+    end
+
+    def name=(n)
+      super(n&.downcase)
+    end
+
+    def owner=(o)
+      super(o&.downcase)
+    end
+
+    def http_url
+      Shipit.github.url("#{owner}/#{name}")
+    end
+
+    def git_url
+      "https://#{Shipit.github.domain}/#{owner}/#{name}.git"
+    end
+  end
+end

app/models/shipit/stack.rb

@@ -22,8 +22,6 @@ def blank?
       end
     end
 
-    REPO_OWNER_MAX_SIZE = 39
-    REPO_NAME_MAX_SIZE = 100
     ENVIRONMENT_MAX_SIZE = 50
     REQUIRED_HOOKS = %i(push status).freeze
 
@@ -40,6 +38,12 @@ def blank?
     has_many :hooks, dependent: :destroy
     has_many :api_clients, dependent: :destroy
     belongs_to :lock_author, class_name: :User, optional: true
+    belongs_to :repository
+    validates_associated :repository
+
+    def repository
+      super || build_repository
+    end
 
     def lock_author(*)
       super || AnonymousUser.new
@@ -51,7 +55,7 @@ def lock_author=(user)
 
     def self.repo(full_name)
       repo_owner, repo_name = full_name.downcase.split('/')
-      where(repo_owner: repo_owner, repo_name: repo_name)
+      Repository.find_by(owner: repo_owner, name: repo_name).stacks
     end
 
     before_validation :update_defaults
@@ -66,11 +70,8 @@ def self.repo(full_name)
     after_commit :sync_github, on: :create
     after_commit :schedule_merges_if_necessary, on: :update
 
-    validates :repo_name, uniqueness: {scope: %i(repo_owner environment), case_sensitive: false,
-                                       message: 'cannot be used more than once with this environment'}
-    validates :repo_owner, :repo_name, :environment, presence: true, ascii_only: true
-    validates :repo_owner, format: {with: /\A[a-z0-9_\-\.]+\z/}, length: {maximum: REPO_OWNER_MAX_SIZE}
-    validates :repo_name, format: {with: /\A[a-z0-9_\-\.]+\z/}, length: {maximum: REPO_NAME_MAX_SIZE}
+    validates :repository, uniqueness: {scope: %i(environment), case_sensitive: false,
+                                        message: 'cannot be used more than once with this environment'}
     validates :environment, format: {with: /\A[a-z0-9\-_\:]+\z/}, length: {maximum: ENVIRONMENT_MAX_SIZE}
     validates :deploy_url, format: {with: URI.regexp(%w(http https ssh))}, allow_blank: true
 
@@ -321,21 +322,12 @@ def merge_method
       cached_deploy_spec&.pull_request_merge_method || Shipit.default_merge_method
     end
 
-    def repo_name=(name)
-      super(name&.downcase)
-    end
-
-    def repo_owner=(name)
-      super(name&.downcase)
-    end
-
-    def repo_http_url
-      Shipit.github.url("#{repo_owner}/#{repo_name}")
-    end
-
-    def repo_git_url
-      "https://#{Shipit.github.domain}/#{repo_owner}/#{repo_name}.git"
-    end
+    delegate :name=, to: :repository, prefix: :repo
+    delegate :name, to: :repository, prefix: :repo
+    delegate :owner=, to: :repository, prefix: :repo
+    delegate :owner, to: :repository, prefix: :repo
+    delegate :http_url, to: :repository, prefix: :repo
+    delegate :git_url, to: :repository, prefix: :repo
 
     def base_path
       Rails.root.join('data', 'stacks', repo_owner, repo_name, environment)
@@ -390,7 +382,7 @@ def refresh_repository!
       if resource.try(:message) == 'Moved Permanently'
         resource = Shipit.github.api.get(resource.url)
       end
-      update!(repo_owner: resource.owner.login, repo_name: resource.name)
+      repository.update!(owner: resource.owner.login, name: resource.name)
     end
 
     def active_task?
@@ -430,11 +422,14 @@ def self.run_deploy_in_foreground(stack:, revision:)
 
     def self.from_param!(param)
       repo_owner, repo_name, environment = param.split('/')
-      where(
-        repo_owner: repo_owner.downcase,
-        repo_name: repo_name.downcase,
-        environment: environment,
-      ).first!
+      includes(:repository)
+        .where(
+          repositories: {
+            owner: repo_owner.downcase,
+            name: repo_name.downcase,
+          },
+          environment: environment,
+        ).first!
     end
 
     delegate :plugins, :task_definitions, :hidden_statuses, :required_statuses, :soft_failing_statuses,

db/migrate/20191209231045_create_shipit_repositories.rb

@@ -0,0 +1,12 @@
+class CreateShipitRepositories < ActiveRecord::Migration[6.0]
+  def change
+    create_table :repositories do |t|
+      t.string :owner, limit: 100, null: false
+      t.string :name, limit: 39, null: false
+
+      t.timestamps
+    end
+
+    add_index :repositories, ["owner", "name"], name: "repository_unicity", unique: true
+  end
+end

db/migrate/20191209231307_add_repository_reference_to_stacks.rb

@@ -0,0 +1,15 @@
+class AddRepositoryReferenceToStacks < ActiveRecord::Migration[6.0]
+  def up
+    change_table(:stacks) do |t|
+      t.references :repository
+    end
+  end
+
+  def down
+    change_column :stacks, :repo_name, :string, null: false
+    change_column :stacks, :repo_owner, :string, null: false
+    change_table(:stacks) do |t|
+      t.remove_references :repository
+    end
+  end
+end

db/migrate/20191216162728_backfill_repository_data.rb

@@ -0,0 +1,20 @@
+class BackfillRepositoryData < ActiveRecord::Migration[6.0]
+  def up
+    repositories = Shipit::Stack.distinct.select(:repo_owner, :repo_name).pluck(:repo_owner, :repo_name)
+    repositories.each do |repo_owner, repo_name|
+      repository = Shipit::Repository.create_or_find_by(
+        owner: repo_owner,
+        name: repo_name,
+      )
+
+      stacks = Shipit::Stack.where(repo_owner: repository.owner, repo_name: repository.name)
+      stacks.update(repository: repository)
+    end
+  end
+
+  def down
+    Shipit::Repository.find_each do |repository|
+      repository.stacks.update_all(repo_owner: repository.owner, repo_name: repository.name)
+    end
+  end
+end

db/migrate/20191216163010_remove_repository_information_from_stacks.rb

@@ -0,0 +1,20 @@
+class RemoveRepositoryInformationFromStacks < ActiveRecord::Migration[6.0]
+  def up
+    change_column :stacks, :repository_id, :integer, null: false
+    change_table(:stacks) do |t|
+      t.remove_index ["repo_owner", "repo_name", "environment"]
+      t.remove :repo_owner
+      t.remove :repo_name
+      t.index ["repository_id", "environment"], name: "stack_unicity", unique: true
+    end
+  end
+
+  def down
+    change_table(:stacks) do |t|
+      t.column :repo_name, :string, limit: 100
+      t.column :repo_owner, :string, limit: 39
+      t.remove_index ["repository_id", "environment"]
+      t.index ["repo_owner", "repo_name", "environment"], name: "stack_unicity", unique: true
+    end
+  end
+end

test/controllers/api/ccmenu_controller_test.rb

@@ -43,7 +43,7 @@ class CCMenuControllerTest < ActionController::TestCase
       end
 
       test "stacks with no deploys render correctly" do
-        stack = Stack.create!(repo_owner: 'foo', repo_name: 'bar')
+        stack = Stack.create!(repository: Repository.new(owner: "foo", name: "bar"))
         get :show, params: {stack_id: stack.to_param}
         assert_payload 'lastBuildStatus', 'Success'
       end

test/controllers/api/stacks_controller_test.rb

@@ -25,7 +25,7 @@ class StacksControllerTest < ActionController::TestCase
           post :create, params: {repo_owner: 'some', repo_name: 'owner/path'}
         end
         assert_response :unprocessable_entity
-        assert_json 'errors', 'repo_name' => ['is invalid']
+        assert_json 'errors', 'repository' => ['is invalid']
       end
 
       test "#create creates a stack and renders it back" do
@@ -38,19 +38,25 @@ class StacksControllerTest < ActionController::TestCase
       end
 
       test "#create fails to create stack if it already exists" do
-        Stack.create!(
-          repo_name: 'rails',
-          repo_owner: 'rails',
+        repository = shipit_repositories(:rails)
+        existing_stack = Stack.create!(
+          repository: repository,
           environment: 'staging',
           branch: 'staging',
         )
 
         assert_no_difference -> { Stack.count } do
-          post :create, params: {repo_name: 'rails', repo_owner: 'rails', environment: 'staging', branch: 'staging'}
+          post :create,
+               params: {
+                 repo_name: existing_stack.repo_name,
+                 repo_owner: existing_stack.repo_owner,
+                 environment: existing_stack.environment,
+                 branch: existing_stack.branch,
+               }
         end
 
         assert_response :unprocessable_entity
-        assert_json 'errors', 'repo_name' => ['cannot be used more than once with this environment']
+        assert_json 'errors', 'repository' => ['cannot be used more than once with this environment']
       end
 
       test "#index returns a list of stacks" do

test/controllers/merge_status_controller_test.rb

@@ -37,13 +37,11 @@ class MergeStatusControllerTest < ActionController::TestCase
     test "GET show prefers stacks with merge_queue_enabled" do
       existing = shipit_stacks(:shipit)
       Shipit::Stack.where(
-        repo_owner: existing.repo_owner,
-        repo_name: existing.repo_name,
+        repository: existing.repository,
       ).update_all(merge_queue_enabled: false)
 
       Shipit::Stack.create(
-        repo_owner: existing.repo_owner,
-        repo_name: existing.repo_name,
+        repository: existing.repository,
         environment: 'foo',
         branch: existing.branch,
         merge_queue_enabled: true,