Merge branch 'refactor-build-service' into 'master'

Refactor Ci::Commit and Ci::Build to have all builds for same :sha on single page

This makes Ci::Commit to have only :sha and simplifies routing to have only :sha in path. The :ref and :push_data is now parameter of Ci::Build.

All commit related data (git author, message and .gitlab-ci.yml) is read directly from repository.

All code related for creating builds is moved to CreateBuildsService.

Status deduction is rewritten to make if more efficient and easier to integrate with Commit Status API.

This is partially working, tests are not yet touched.

This slightly changes view of Commit:
![Screen_Shot_2015-10-02_at_15.21.47](https://gitlab.com/gitlab-org/gitlab-ce/uploads/ad3f1ccdcc87659ea437d8db6c5b9f94/Screen_Shot_2015-10-02_at_15.21.47.png)

@dzaporozhets What do you think?


See merge request !1502

app/controllers/ci/builds_controller.rb

@@ -18,7 +18,7 @@ module Ci
         if commit
           # Redirect to commit page
-          redirect_to ci_project_ref_commit_path(@project, @build.commit.ref, @build.commit.sha)
+          redirect_to ci_project_commit_path(@project, @build.commit)
           return
         end
       end

app/controllers/ci/commits_controller.rb

@@ -13,7 +13,7 @@ module Ci
     end
     def status
-      commit = Ci::Project.find(params[:project_id]).commits.find_by_sha_and_ref!(params[:id], params[:ref_id])
+      commit = Ci::Project.find(params[:project_id]).commits.find_by_sha!(params[:id])
       render json: commit.to_json(only: [:id, :sha], methods: [:status, :coverage])
     rescue ActiveRecord::RecordNotFound
       render json: { status: "not_found" }
@@ -22,7 +22,7 @@ module Ci
     def cancel
       commit.builds.running_or_pending.each(&:cancel)
-      redirect_to ci_project_ref_commits_path(project, commit.ref, commit.sha)
+      redirect_to ci_project_commits_path(project, commit.sha)
     end
     private
@@ -32,7 +32,7 @@ module Ci
     end
     def commit
-      @commit ||= Ci::Project.find(params[:project_id]).commits.find_by_sha_and_ref!(params[:id], params[:ref_id])
+      @commit ||= Ci::Project.find(params[:project_id]).commits.find_by_sha!(params[:id])
     end
   end
 end

app/controllers/ci/projects_controller.rb

@@ -19,7 +19,8 @@ module Ci
       @ref = params[:ref]
       @commits = @project.commits.reverse_order
-      @commits = @commits.where(ref: @ref) if @ref
+      # TODO: this is broken
+      # @commits = @commits.where(ref: @ref) if @ref
       @commits = @commits.page(params[:page]).per(20)
     end

app/helpers/builds_helper.rb

@@ -3,10 +3,6 @@ module BuildsHelper
     gitlab_ref_link build.project, build.ref
   end
-  def build_compare_link build
-    gitlab_compare_link build.project, build.commit.short_before_sha, build.short_sha
-  end
-
   def build_commit_link build
     gitlab_commit_link build.project, build.short_sha
   end

app/helpers/ci/commits_helper.rb

 module Ci
   module CommitsHelper
     def ci_commit_path(commit)
-      ci_project_ref_commits_path(commit.project, commit.ref, commit.sha)
+      ci_project_commits_path(commit.project, commit)
     end
     def commit_link(commit)

app/helpers/ci/gitlab_helper.rb

@@ -26,7 +26,7 @@ module Ci
     def yaml_web_editor_link(project)
       commits = project.commits
-      if commits.any? && commits.last.push_data[:ci_yaml_file]
+      if commits.any? && commits.last.ci_yaml_file
         "#{project.gitlab_url}/edit/master/.gitlab-ci.yml"
       else
         "#{project.gitlab_url}/new/master"

app/helpers/ci_status_helper.rb

 module CiStatusHelper
   def ci_status_path(ci_commit)
-    ci_project_ref_commits_path(ci_commit.project, ci_commit.ref, ci_commit)
+    ci_project_commits_path(ci_commit.project, ci_commit)
   end
   def ci_status_icon(ci_commit)

app/models/ci/build.rb

@@ -32,12 +32,14 @@ module Ci
     belongs_to :commit, class_name: 'Ci::Commit'
     belongs_to :runner, class_name: 'Ci::Runner'
     belongs_to :trigger_request, class_name: 'Ci::TriggerRequest'
+    belongs_to :user
     serialize :options
     validates :commit, presence: true
     validates :status, presence: true
     validates :coverage, numericality: true, allow_blank: true
+    validates_presence_of :ref
     scope :running, ->() { where(status: "running") }
     scope :pending, ->() { where(status: "pending") }
@@ -45,6 +47,10 @@ module Ci
     scope :failed, ->() { where(status: "failed")  }
     scope :unstarted, ->() { where(runner_id: nil) }
     scope :running_or_pending, ->() { where(status:[:running, :pending]) }
+    scope :latest, ->() { where(id: unscope(:select).select('max(id)').group(:name)).order(stage_idx: :asc) }
+    scope :ignore_failures, ->() { where(allow_failure: false) }
+    scope :for_ref, ->(ref) { where(ref: ref) }
+    scope :similar, ->(build) { where(ref: build.ref, tag: build.tag, trigger_request_id: build.trigger_request_id) }
     acts_as_taggable
@@ -75,6 +81,8 @@ module Ci
       def retry(build)
         new_build = Ci::Build.new(status: :pending)
+        new_build.ref = build.ref
+        new_build.tag = build.tag
         new_build.options = build.options
         new_build.commands = build.commands
         new_build.tag_list = build.tag_list
@@ -82,6 +90,7 @@ module Ci
         new_build.name = build.name
         new_build.allow_failure = build.allow_failure
         new_build.stage = build.stage
+        new_build.stage_idx = build.stage_idx
         new_build.trigger_request = build.trigger_request
         new_build.save
         new_build
@@ -117,8 +126,8 @@ module Ci
           Ci::WebHookService.new.build_end(build)
         end
-        if build.commit.success?
-          build.commit.create_next_builds(build.trigger_request)
+        if build.commit.should_create_next_builds?(build)
+          build.commit.create_next_builds(build.ref, build.tag, build.user, build.trigger_request)
         end
         project.execute_services(build)
@@ -135,9 +144,13 @@ module Ci
       state :canceled, value: 'canceled'
     end
-    delegate :sha, :short_sha, :before_sha, :ref, :project,
+    delegate :sha, :short_sha, :project,
       to: :commit, prefix: false
+    def before_sha
+      Gitlab::Git::BLANK_SHA
+    end
+
     def trace_html
       html = Ci::Ansi2html::convert(trace) if trace.present?
       html || ''
@@ -187,6 +200,16 @@ module Ci
       project.name
     end
+    def project_recipients
+      recipients = project.email_recipients.split(' ')
+
+      if project.email_add_pusher? && user.present? && user.notification_email.present?
+        recipients << user.notification_email
+      end
+
+      recipients.uniq
+    end
+
     def repo_url
       project.repo_url_with_auth
     end

app/models/ci/commit.rb

@@ -23,9 +23,7 @@ module Ci
     has_many :builds, dependent: :destroy, class_name: 'Ci::Build'
     has_many :trigger_requests, dependent: :destroy, class_name: 'Ci::TriggerRequest'
-    serialize :push_data
-
-    validates_presence_of :ref, :sha, :before_sha, :push_data
+    validates_presence_of :sha
     validate :valid_commit_sha
     def self.truncate_sha(sha)
@@ -60,28 +58,16 @@ module Ci
       end
     end
-    def new_branch?
-      before_sha == Ci::Git::BLANK_SHA
-    end
-
-    def compare?
-      !new_branch?
-    end
-
     def git_author_name
-      commit_data[:author][:name] if commit_data && commit_data[:author]
+      commit_data.author_name if commit_data
     end
     def git_author_email
-      commit_data[:author][:email] if commit_data && commit_data[:author]
+      commit_data.author_email if commit_data
     end
     def git_commit_message
-      commit_data[:message] if commit_data && commit_data[:message]
-    end
-
-    def short_before_sha
-      Ci::Commit.truncate_sha(before_sha)
+      commit_data.message if commit_data
     end
     def short_sha
@@ -89,84 +75,51 @@ module Ci
     end
     def commit_data
-      push_data[:commits].find do |commit|
-        commit[:id] == sha
-      end
+      @commit ||= gl_project.commit(sha)
     rescue
       nil
     end
-    def project_recipients
-      recipients = project.email_recipients.split(' ')
-
-      if project.email_add_pusher? && push_data[:user_email].present?
-        recipients << push_data[:user_email]
-      end
-
-      recipients.uniq
-    end
-
     def stage
-      return unless config_processor
-      stages = builds_without_retry.select(&:active?).map(&:stage)
-      config_processor.stages.find { |stage| stages.include? stage }
+      running_or_pending = builds_without_retry.running_or_pending
+      running_or_pending.limit(1).pluck(:stage).first
     end
-    def create_builds_for_stage(stage, trigger_request)
+    def create_builds(ref, tag, user, trigger_request = nil)
       return if skip_ci? && trigger_request.blank?
       return unless config_processor
-
-      builds_attrs = config_processor.builds_for_stage_and_ref(stage, ref, tag)
-      builds_attrs.map do |build_attrs|
-        builds.create!({
-                         name: build_attrs[:name],
-                         commands: build_attrs[:script],
-                         tag_list: build_attrs[:tags],
-                         options: build_attrs[:options],
-                         allow_failure: build_attrs[:allow_failure],
-                         stage: build_attrs[:stage],
-                         trigger_request: trigger_request,
-                       })
+      config_processor.stages.any? do |stage|
+        CreateBuildsService.new.execute(self, stage, ref, tag, user, trigger_request).present?
       end
     end
-    def create_next_builds(trigger_request)
+    def create_next_builds(ref, tag, user, trigger_request)
       return if skip_ci? && trigger_request.blank?
       return unless config_processor
-      stages = builds.where(trigger_request: trigger_request).group_by(&:stage)
+      stages = builds.where(ref: ref, tag: tag, trigger_request: trigger_request).group_by(&:stage)
       config_processor.stages.any? do |stage|
-        !stages.include?(stage) && create_builds_for_stage(stage, trigger_request).present?
+        unless stages.include?(stage)
+          CreateBuildsService.new.execute(self, stage, ref, tag, user, trigger_request).present?
+        end
       end
     end
-    def create_builds(trigger_request = nil)
-      return if skip_ci? && trigger_request.blank?
-      return unless config_processor
-      config_processor.stages.any? do |stage|
-        create_builds_for_stage(stage, trigger_request).present?
-      end
+    def refs
+      builds.group(:ref).pluck(:ref)
+    end
+    def last_ref
+      builds.latest.first.try(:ref)
     end
     def builds_without_retry
-      @builds_without_retry ||=
-        begin
-          grouped_builds = builds.group_by(&:name)
-          grouped_builds.map do |name, builds|
-            builds.sort_by(&:id).last
-          end
-        end
+      builds.latest
     end
-    def builds_without_retry_sorted
-      return builds_without_retry unless config_processor
-
-      stages = config_processor.stages
-      builds_without_retry.sort_by do |build|
-        [stages.index(build.stage) || -1, build.name || ""]
-      end
+    def builds_without_retry_for_ref(ref)
+      builds.for_ref(ref).latest
     end
     def retried_builds
@@ -225,6 +178,10 @@ module Ci
       @duration ||= builds_without_retry.select(&:duration).sum(&:duration).to_i
     end
+    def duration_for_ref(ref)
+      builds_without_retry_for_ref(ref).select(&:duration).sum(&:duration).to_i
+    end
+
     def finished_at
       @finished_at ||= builds.order('finished_at DESC').first.try(:finished_at)
     end
@@ -238,12 +195,12 @@ module Ci
       end
     end
-    def matrix?
-      builds_without_retry.size > 1
+    def matrix_for_ref?(ref)
+      builds_without_retry_for_ref(ref).pluck(:id).size > 1
     end
     def config_processor
-      @config_processor ||= Ci::GitlabCiYamlProcessor.new(push_data[:ci_yaml_file])
+      @config_processor ||= Ci::GitlabCiYamlProcessor.new(ci_yaml_file)
     rescue Ci::GitlabCiYamlProcessor::ValidationError => e
       save_yaml_error(e.message)
       nil
@@ -253,16 +210,31 @@ module Ci
       nil
     end
+    def ci_yaml_file
+      gl_project.repository.blob_at(sha, '.gitlab-ci.yml')
+    rescue
+      nil
+    end
+
     def skip_ci?
       return false if builds.any?
-      commits = push_data[:commits]
-      commits.present? && commits.last[:message] =~ /(\[ci skip\])/
+      git_commit_message =~ /(\[ci skip\])/ if git_commit_message
     end
     def update_committed!
       update!(committed_at: DateTime.now)
     end
+    def should_create_next_builds?(build)
+      # don't create other builds if this one is retried
+      other_builds = builds.similar(build).latest
+      return false unless other_builds.include?(build)
+
+      other_builds.all? do |build|
+        build.success? || build.ignored?
+      end
+    end
+
     private
     def save_yaml_error(error)

app/models/ci/project_status.rb

@@ -28,18 +28,6 @@ module Ci
       status
     end
-    # only check for toggling build status within same ref.
-    def last_commit_changed_status?
-      ref = last_commit.ref
-      last_commits = commits.where(ref: ref).last(2)
-
-      if last_commits.size < 2
-        false
-      else
-        last_commits[0].status != last_commits[1].status
-      end
-    end
-
     def last_commit_for_ref(ref)
       commits.where(ref: ref).last
     end

app/models/project.rb

@@ -744,7 +744,11 @@ class Project < ActiveRecord::Base
   end
   def ci_commit(sha)
-    gitlab_ci_project.commits.find_by(sha: sha) if gitlab_ci?
+    ci_commits.find_by(sha: sha)
+  end
+
+  def ensure_ci_commit(sha)
+    ci_commit(sha) || ci_commits.create(sha: sha)
   end
   def ensure_gitlab_ci_project

app/models/project_services/ci/hip_chat_message.rb

@@ -11,14 +11,7 @@ module Ci
     def to_s
       lines = Array.new
       lines.push("<a href=\"#{ci_project_url(project)}\">#{project.name}</a> - ")
-      
-      if commit.matrix?
-        lines.push("<a href=\"#{ci_project_ref_commits_url(project, commit.ref, commit.sha)}\">Commit ##{commit.id}</a></br>")
-      else
-        first_build = commit.builds_without_retry.first
-        lines.push("<a href=\"#{ci_project_build_url(project, first_build)}\">Build '#{first_build.name}' ##{first_build.id}</a></br>")
-      end
-      
+      lines.push("<a href=\"#{ci_project_commits_url(project, commit.sha)}\">Commit ##{commit.id}</a></br>")
       lines.push("#{commit.short_sha} #{commit.git_author_name} - #{commit.git_commit_message}</br>")
       lines.push("#{humanized_status(commit_status)} in #{commit.duration} second(s).")
       lines.join('')

app/models/project_services/ci/mail_service.rb

@@ -61,7 +61,7 @@ module Ci
     end
     def execute(build)
-      build.commit.project_recipients.each do |recipient|
+      build.project_recipients.each do |recipient|
         case build.status.to_sym
         when :success
           mailer.build_success_email(build.id, recipient)

app/models/project_services/ci/slack_message.rb

@@ -23,15 +23,13 @@ module Ci
     def attachments
       fields = []
-      if commit.matrix?
-        commit.builds_without_retry.each do |build|
-          next if build.allow_failure?
-          next unless build.failed?
-          fields << {
-            title: build.name,
-            value: "Build <#{ci_project_build_url(project, build)}|\##{build.id}> failed in #{build.duration.to_i} second(s)."
-          }
-        end
+      commit.builds_without_retry.each do |build|
+        next if build.allow_failure?
+        next unless build.failed?
+        fields << {
+          title: build.name,
+          value: "Build <#{ci_project_build_url(project, build)}|\##{build.id}> failed in #{build.duration.to_i} second(s)."
+        }
       end
       [{
@@ -47,12 +45,7 @@ module Ci
     def attachment_message
       out = "<#{ci_project_url(project)}|#{project_name}>: "
-      if commit.matrix?
-        out << "Commit <#{ci_project_ref_commits_url(project, commit.ref, commit.sha)}|\##{commit.id}> "
-      else
-        build = commit.builds_without_retry.first
-        out << "Build <#{ci_project_build_url(project, build)}|\##{build.id}> "
-      end
+      out << "Commit <#{ci_project_commits_url(project, commit.sha)}|\##{commit.id}> "
       out << "(<#{commit_sha_link}|#{commit.short_sha}>) "
       out << "of <#{commit_ref_link}|#{commit.ref}> "
       out << "by #{commit.git_author_name} " if commit.git_author_name

app/models/project_services/gitlab_ci_service.rb

@@ -40,19 +40,10 @@ class GitlabCiService < CiService
   def execute(data)
     return unless supported_events.include?(data[:object_kind])
-    sha = data[:checkout_sha]
-
-    if sha.present?
-      file = ci_yaml_file(sha)
-
-      if file && file.data
-        data.merge!(ci_yaml_file: file.data)
-      end
-    end
-
-    ci_project = Ci::Project.find_by(gitlab_id: project.id)
+    ci_project = project.gitlab_ci_project
     if ci_project
-      Ci::CreateCommitService.new.execute(ci_project, data)
+      current_user = User.find_by(id: data[:user_id])
+      Ci::CreateCommitService.new.execute(ci_project, current_user, data)
     end
   end
@@ -63,7 +54,7 @@ class GitlabCiService < CiService
   end
   def get_ci_commit(sha, ref)
-    Ci::Project.find(project.gitlab_ci_project).commits.find_by_sha_and_ref!(sha, ref)
+    Ci::Project.find(project.gitlab_ci_project).commits.find_by_sha!(sha)
   end
   def commit_status(sha, ref)
@@ -80,7 +71,7 @@ class GitlabCiService < CiService
   def build_page(sha, ref)
     if project.gitlab_ci_project.present?
-      ci_project_ref_commits_url(project.gitlab_ci_project, ref, sha)
+      ci_project_commits_url(project.gitlab_ci_project, sha)
     end
   end
@@ -99,14 +90,4 @@ class GitlabCiService < CiService
   def fields
     []
   end
-
-  private
-
-  def ci_yaml_file(sha)
-    repository.blob_at(sha, '.gitlab-ci.yml')
-  end
-
-  def repository
-    project.repository
-  end
 end

app/models/user.rb

@@ -130,6 +130,7 @@ class User < ActiveRecord::Base
   has_many :assigned_merge_requests,  dependent: :destroy, foreign_key: :assignee_id, class_name: "MergeRequest"
   has_many :oauth_applications, class_name: 'Doorkeeper::Application', as: :owner, dependent: :destroy
   has_one  :abuse_report,             dependent: :destroy
+  has_many :ci_builds,                dependent: :nullify, class_name: 'Ci::Build'
   #

app/services/ci/create_builds_service.rb

+module Ci
+  class CreateBuildsService
+    def execute(commit, stage, ref, tag, user, trigger_request)
+      builds_attrs = commit.config_processor.builds_for_stage_and_ref(stage, ref, tag)
+
+      builds_attrs.map do |build_attrs|
+        # don't create the same build twice
+        unless commit.builds.find_by(ref: ref, tag: tag, trigger_request: trigger_request, name: build_attrs[:name])
+          build_attrs.slice!(:name,
+                             :commands,
+                             :tag_list,
+                             :options,
+                             :allow_failure,
+                             :stage,
+                             :stage_idx)
+
+          build_attrs.merge!(ref: ref,
+                             tag: tag,
+                             trigger_request: trigger_request,
+                             user: user)
+
+          commit.builds.create!(build_attrs)
+        end
+      end
+    end
+  end
+end

app/services/ci/create_commit_service.rb

 module Ci
   class CreateCommitService
-    def execute(project, params)
-      before_sha = params[:before]
+    def execute(project, user, params)
       sha = params[:checkout_sha] || params[:after]
       origin_ref = params[:ref]
       
@@ -16,33 +15,10 @@ module Ci
         return false
       end
-      commit = project.commits.find_by_sha_and_ref(sha, ref)
-
-      # Create commit if not exists yet
-      unless commit
-        data = {
-          ref: ref,
-          sha: sha,
-          tag: origin_ref.start_with?('refs/tags/'),
-          before_sha: before_sha,
-          push_data: {
-            before: before_sha,
-            after: sha,
-            ref: ref,
-            user_name: params[:user_name],
-            user_email: params[:user_email],
-            repository: params[:repository],
-            commits: params[:commits],
-            total_commits_count: params[:total_commits_count],
-            ci_yaml_file: params[:ci_yaml_file]
-          }
-        }
-
-        commit = project.commits.create(data)
-      end
-
+      tag = origin_ref.start_with?('refs/tags/')
+      commit = project.gl_project.ensure_ci_commit(sha)
       commit.update_committed!
-      commit.create_builds unless commit.builds.any?
+      commit.create_builds(ref, tag, user)
       commit
     end

app/services/ci/create_trigger_request_service.rb

 module Ci
   class CreateTriggerRequestService
     def execute(project, trigger, ref, variables = nil)
-      commit = project.commits.where(ref: ref).last
+      commit = project.gl_project.commit(ref)
       return unless commit
+      # check if ref is tag
+      tag = project.gl_project.repository.find_tag(ref).present?
+
+      ci_commit = project.gl_project.ensure_ci_commit(commit.sha)
+
       trigger_request = trigger.trigger_requests.create!(
-        commit: commit,
-        variables: variables
+        variables: variables,
+        commit: ci_commit,
       )
-      if commit.create_builds(trigger_request)
+      if ci_commit.create_builds(ref, tag, nil, trigger_request)
         trigger_request
       end
     end

app/services/ci/web_hook_service.rb

@@ -27,9 +27,8 @@ module Ci
         project_name: project.name,
         gitlab_url: project.gitlab_url,
         ref: build.ref,
-        sha: build.sha,
         before_sha: build.before_sha,
-        push_data: build.commit.push_data
+        sha: build.sha,
       })
     end
   end