Elevate the "Repository" concept

Summary
-------

At PowerHRG we desire to implement the ability to automatically deploy
a branch as a new "review instance" of our application to our
Kubernetes cluster when a Pull Request for that branch is opened - and
subsequently clean up the instance when the PR is closed or merged -
functionality similar to Heroku's Review Apps. However, we maintain a
number of projects and do NOT want to expose this functionality for
every repository.

As we understand it, Shipit offers no mechanism at the repository
level to control which repositories can dynamically provision
stacks. It seems the Stack is the highest level concept which involves
a repository, but this remains insufficient since the Stack also
marries a particular branch and environment to the repository
concept. What we _think_ we'd like is a higher level concept which
represents the repository to which stacks could belong. This
repository could then contain its own configuration for the
project-level features we desire.

This change set extracts the repository data from the stack concept
while retaining the original stack API - all repository related
functionality of the Stack is now delegated to the stack's Repository
object.

References
----------

- Shopify#960
- Shopify#963

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,