diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 53af87a271ad2f6b88d74893cdf428992a81c279..5eb30f4aaa05e48ab456fb0ea0a40d9f2dd77596 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -7,12 +7,17 @@ module Ci include Presentable include Gitlab::OptimisticLocking include Gitlab::Utils::StrongMemoize + include AtomicInternalId belongs_to :project, inverse_of: :pipelines belongs_to :user belongs_to :auto_canceled_by, class_name: 'Ci::Pipeline' belongs_to :pipeline_schedule, class_name: 'Ci::PipelineSchedule' + has_internal_id :iid, scope: :project, presence: false, init: ->(s) do + s&.project&.pipelines&.maximum(:iid) || s&.project&.pipelines&.count + end + has_many :stages has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline has_many :builds, foreign_key: :commit_id, inverse_of: :pipeline @@ -531,6 +536,7 @@ module Ci def predefined_variables Gitlab::Ci::Variables::Collection.new + .append(key: 'CI_PIPELINE_IID', value: iid.to_s) .append(key: 'CI_CONFIG_PATH', value: ci_yaml_file_path) .append(key: 'CI_PIPELINE_SOURCE', value: source.to_s) .append(key: 'CI_COMMIT_MESSAGE', value: git_commit_message) diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb index 22f516a172fea422f66da73bb478797d0358a575..164c704260e9268c989dfe8378a472701e5417ad 100644 --- a/app/models/concerns/atomic_internal_id.rb +++ b/app/models/concerns/atomic_internal_id.rb @@ -25,9 +25,13 @@ module AtomicInternalId extend ActiveSupport::Concern module ClassMethods - def has_internal_id(column, scope:, init:) # rubocop:disable Naming/PredicateName - before_validation(on: :create) do + def has_internal_id(column, scope:, init:, presence: true) # rubocop:disable Naming/PredicateName + before_validation :"ensure_#{scope}_#{column}!", on: :create + validates column, presence: presence + + define_method("ensure_#{scope}_#{column}!") do scope_value = association(scope).reader + if read_attribute(column).blank? && scope_value scope_attrs = { scope_value.class.table_name.singularize.to_sym => scope_value } usage = self.class.table_name.to_sym @@ -35,13 +39,9 @@ module AtomicInternalId new_iid = InternalId.generate_next(self, scope_attrs, usage, init) write_attribute(column, new_iid) end - end - validates column, presence: true, numericality: true + read_attribute(column) + end end end - - def to_param - iid.to_s - end end diff --git a/app/models/concerns/iid_routes.rb b/app/models/concerns/iid_routes.rb new file mode 100644 index 0000000000000000000000000000000000000000..246748cf52cb35788a61454952ecefc8b66728cd --- /dev/null +++ b/app/models/concerns/iid_routes.rb @@ -0,0 +1,9 @@ +module IidRoutes + ## + # This automagically enforces all related routes to use `iid` instead of `id` + # If you want to use `iid` for some routes and `id` for other routes, this module should not to be included, + # instead you should define `iid` or `id` explictly at each route generators. e.g. pipeline_path(project.id, pipeline.iid) + def to_param + iid.to_s + end +end diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 254764eefde1df19bdfe7711c15c31e82359e1c9..ac86e9e8de06398708ecc5030115b5bcf61d86f3 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -1,5 +1,6 @@ class Deployment < ActiveRecord::Base include AtomicInternalId + include IidRoutes belongs_to :project, required: true belongs_to :environment, required: true diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index f7f930e86ed4be792ea8b8c316596366ba0bc355..f50f28deffe0c052b24fdbf9864158c9d7dc9057 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -14,7 +14,7 @@ class InternalId < ActiveRecord::Base belongs_to :project belongs_to :namespace - enum usage: { issues: 0, merge_requests: 1, deployments: 2, milestones: 3, epics: 4 } + enum usage: { issues: 0, merge_requests: 1, deployments: 2, milestones: 3, epics: 4, ci_pipelines: 5 } validates :usage, presence: true diff --git a/app/models/issue.rb b/app/models/issue.rb index 41a290f34b454433febd4eb2c604fb1c87155c14..d136700836df6a5a486e52b29f83089bdf91097a 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -2,6 +2,7 @@ require 'carrierwave/orm/activerecord' class Issue < ActiveRecord::Base include AtomicInternalId + include IidRoutes include Issuable include Noteable include Referable diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 79fc155fd3cab288926e85d207f895a783e53983..4c1628d2bdb33f27eab7c5f76abc63f97add2301 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1,5 +1,6 @@ class MergeRequest < ActiveRecord::Base include AtomicInternalId + include IidRoutes include Issuable include Noteable include Referable diff --git a/app/models/milestone.rb b/app/models/milestone.rb index d14e3a4ded56349ea58ffa8ab724ff54eac74c30..d05dcfd083a61f8c89bf70a164c9141c1697615d 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -9,6 +9,7 @@ class Milestone < ActiveRecord::Base include CacheMarkdownField include AtomicInternalId + include IidRoutes include Sortable include Referable include StripAttribute diff --git a/changelogs/unreleased/per-project-pipeline-iid.yml b/changelogs/unreleased/per-project-pipeline-iid.yml new file mode 100644 index 0000000000000000000000000000000000000000..78a513a9986bcb022ef76caab73a4ea01d01f41e --- /dev/null +++ b/changelogs/unreleased/per-project-pipeline-iid.yml @@ -0,0 +1,5 @@ +--- +title: Add per-project pipeline id +merge_request: 18558 +author: +type: added diff --git a/db/migrate/20180424160449_add_pipeline_iid_to_ci_pipelines.rb b/db/migrate/20180424160449_add_pipeline_iid_to_ci_pipelines.rb new file mode 100644 index 0000000000000000000000000000000000000000..e8f0c91d612a885d9e0ae9abb239679da6df0f93 --- /dev/null +++ b/db/migrate/20180424160449_add_pipeline_iid_to_ci_pipelines.rb @@ -0,0 +1,13 @@ +class AddPipelineIidToCiPipelines < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + add_column :ci_pipelines, :iid, :integer + end + + def down + remove_column :ci_pipelines, :iid, :integer + end +end diff --git a/db/migrate/20180425205249_add_index_constraints_to_pipeline_iid.rb b/db/migrate/20180425205249_add_index_constraints_to_pipeline_iid.rb new file mode 100644 index 0000000000000000000000000000000000000000..3fa59b44d5d79014ea5e590af37c79b05c7e6403 --- /dev/null +++ b/db/migrate/20180425205249_add_index_constraints_to_pipeline_iid.rb @@ -0,0 +1,15 @@ +class AddIndexConstraintsToPipelineIid < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :ci_pipelines, [:project_id, :iid], unique: true, where: 'iid IS NOT NULL' + end + + def down + remove_concurrent_index :ci_pipelines, [:project_id, :iid] + end +end diff --git a/db/schema.rb b/db/schema.rb index 97247387bc7a385468f603a080e16c0d77043b11..0d6b44d1b92d1c792a51d920ad34a4c1e080949c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -451,10 +451,12 @@ ActiveRecord::Schema.define(version: 20180529093006) do t.integer "config_source" t.boolean "protected" t.integer "failure_reason" + t.integer "iid" end add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree add_index "ci_pipelines", ["pipeline_schedule_id"], name: "index_ci_pipelines_on_pipeline_schedule_id", using: :btree + add_index "ci_pipelines", ["project_id", "iid"], name: "index_ci_pipelines_on_project_id_and_iid", unique: true, where: "(iid IS NOT NULL)", using: :btree add_index "ci_pipelines", ["project_id", "ref", "status", "id"], name: "index_ci_pipelines_on_project_id_and_ref_and_status_and_id", using: :btree add_index "ci_pipelines", ["project_id", "sha"], name: "index_ci_pipelines_on_project_id_and_sha", using: :btree add_index "ci_pipelines", ["project_id"], name: "index_ci_pipelines_on_project_id", using: :btree diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index f10423b92cf544ebbd73b5ac321cc1e7ea76ef57..aa4395b01a9f7d86d3c9b2c7485e9c30a51bbd78 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -72,6 +72,7 @@ future GitLab releases.** | **CI_RUNNER_REVISION** | all | 10.6 | GitLab Runner revision that is executing the current job | | **CI_RUNNER_EXECUTABLE_ARCH** | all | 10.6 | The OS/architecture of the GitLab Runner executable (note that this is not necessarily the same as the environment of the executor) | | **CI_PIPELINE_ID** | 8.10 | 0.5 | The unique id of the current pipeline that GitLab CI uses internally | +| **CI_PIPELINE_IID** | 11.0 | all | The unique id of the current pipeline scoped to project | | **CI_PIPELINE_TRIGGERED** | all | all | The flag to indicate that job was [triggered] | | **CI_PIPELINE_SOURCE** | 10.0 | all | Indicates how the pipeline was triggered. Possible options are: `push`, `web`, `trigger`, `schedule`, `api`, and `pipeline`. For pipelines created before GitLab 9.5, this will show as `unknown` | | **CI_PROJECT_DIR** | all | all | The full path where the repository is cloned and where the job is run | @@ -352,6 +353,8 @@ Running on runner-8a2f473d-project-1796893-concurrent-0 via runner-8a2f473d-mach ++ CI_PROJECT_URL=https://example.com/gitlab-examples/ci-debug-trace ++ export CI_PIPELINE_ID=52666 ++ CI_PIPELINE_ID=52666 +++ export CI_PIPELINE_IID=123 +++ CI_PIPELINE_IID=123 ++ export CI_RUNNER_ID=1337 ++ CI_RUNNER_ID=1337 ++ export CI_RUNNER_DESCRIPTION=shared-runners-manager-1.example.com @@ -439,6 +442,7 @@ export CI_JOB_MANUAL="true" export CI_JOB_TRIGGERED="true" export CI_JOB_TOKEN="abcde-1234ABCD5678ef" export CI_PIPELINE_ID="1000" +export CI_PIPELINE_IID="10" export CI_PROJECT_ID="34" export CI_PROJECT_DIR="/builds/gitlab-org/gitlab-ce" export CI_PROJECT_NAME="gitlab-ce" diff --git a/lib/gitlab/ci/pipeline/chain/populate.rb b/lib/gitlab/ci/pipeline/chain/populate.rb index 69b8a8fc68ff662746dfdf58feadaae0effce6ff..f34c11ca3c2677a697cbacc65ab0513113431145 100644 --- a/lib/gitlab/ci/pipeline/chain/populate.rb +++ b/lib/gitlab/ci/pipeline/chain/populate.rb @@ -8,6 +8,9 @@ module Gitlab PopulateError = Class.new(StandardError) def perform! + # Allocate next IID. This operation must be outside of transactions of pipeline creations. + pipeline.ensure_project_iid! + ## # Populate pipeline with block argument of CreatePipelineService#execute. # diff --git a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb index 4d7d6951a519ed9a1b8394ca0f0e29f448e193db..c5a4d9b477862d264391193bdf6776c1ba2f8c28 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb @@ -42,6 +42,10 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do it 'correctly assigns user' do expect(pipeline.builds).to all(have_attributes(user: user)) end + + it 'has pipeline iid' do + expect(pipeline.iid).to be > 0 + end end context 'when pipeline is empty' do @@ -68,6 +72,10 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do expect(pipeline.errors.to_a) .to include 'No stages / jobs for this pipeline.' end + + it 'wastes pipeline iid' do + expect(InternalId.ci_pipelines.where(project_id: project.id).last.last_value).to be > 0 + end end context 'when pipeline has validation errors' do @@ -87,6 +95,10 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do expect(pipeline.errors.to_a) .to include 'Failed to build the pipeline!' end + + it 'wastes pipeline iid' do + expect(InternalId.ci_pipelines.where(project_id: project.id).last.last_value).to be > 0 + end end context 'when there is a seed blocks present' do @@ -111,6 +123,12 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do expect(pipeline.variables.first.key).to eq 'VAR' expect(pipeline.variables.first.value).to eq '123' end + + it 'has pipeline iid' do + step.perform! + + expect(pipeline.iid).to be > 0 + end end context 'when seeds block tries to persist some resources' do @@ -121,6 +139,12 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do it 'raises exception' do expect { step.perform! }.to raise_error(ActiveRecord::RecordNotSaved) end + + it 'wastes pipeline iid' do + expect { step.perform! }.to raise_error + + expect(InternalId.ci_pipelines.where(project_id: project.id).last.last_value).to be > 0 + end end end @@ -132,22 +156,39 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do end end - context 'when using only/except build policies' do - let(:config) do - { rspec: { script: 'rspec', stage: 'test', only: ['master'] }, - prod: { script: 'cap prod', stage: 'deploy', only: ['tags'] } } - end + context 'when variables policy is specified' do + shared_examples_for 'a correct pipeline' do + it 'populates pipeline according to used policies' do + step.perform! - let(:pipeline) do - build(:ci_pipeline, ref: 'master', config: config) + expect(pipeline.stages.size).to eq 1 + expect(pipeline.stages.first.builds.size).to eq 1 + expect(pipeline.stages.first.builds.first.name).to eq 'rspec' + end end - it 'populates pipeline according to used policies' do - step.perform! + context 'when using only/except build policies' do + let(:config) do + { rspec: { script: 'rspec', stage: 'test', only: ['master'] }, + prod: { script: 'cap prod', stage: 'deploy', only: ['tags'] } } + end + + let(:pipeline) do + build(:ci_pipeline, ref: 'master', config: config) + end - expect(pipeline.stages.size).to eq 1 - expect(pipeline.stages.first.builds.size).to eq 1 - expect(pipeline.stages.first.builds.first.name).to eq 'rspec' + it_behaves_like 'a correct pipeline' + + context 'when variables expression is specified' do + context 'when pipeline iid is the subject' do + let(:config) do + { rspec: { script: 'rspec', only: { variables: ["$CI_PIPELINE_IID == '1'"] } }, + prod: { script: 'cap prod', only: { variables: ["$CI_PIPELINE_IID == '1000'"] } } } + end + + it_behaves_like 'a correct pipeline' + end + end end end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 74e7a45fd6ca5c8d215f6791d20c0a872739cfcb..2aebdc57f7cd5d73143dc3ce23d5a0cac0dc42f2 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -242,6 +242,7 @@ Ci::Pipeline: - config_source - failure_reason - protected +- iid Ci::Stage: - id - name diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index b5eac913639e1b4ff8583e8c8229be7b5a81458e..66c9708b4cfa6b833111a07ff329114cecdbf02e 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1559,6 +1559,7 @@ describe Ci::Build do { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true }, { key: 'CI_PROJECT_URL', value: project.web_url, public: true }, { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true }, + { key: 'CI_PIPELINE_IID', value: pipeline.iid.to_s, public: true }, { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true }, { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true }, { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true }, diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index f5295bec65b1ffc4d1f340875577b83e33caff49..24692ebb9a3b701ab6bf1c6024ec2c142d3504a4 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -35,6 +35,16 @@ describe Ci::Pipeline, :mailer do end end + describe 'modules' do + it_behaves_like 'AtomicInternalId', validate_presence: false do + let(:internal_id_attribute) { :iid } + let(:instance) { build(:ci_pipeline) } + let(:scope) { :project } + let(:scope_attrs) { { project: instance.project } } + let(:usage) { :ci_pipelines } + end + end + describe '#source' do context 'when creating new pipeline' do let(:pipeline) do @@ -195,7 +205,8 @@ describe Ci::Pipeline, :mailer do it 'includes all predefined variables in a valid order' do keys = subject.map { |variable| variable[:key] } - expect(keys).to eq %w[CI_CONFIG_PATH + expect(keys).to eq %w[CI_PIPELINE_IID + CI_CONFIG_PATH CI_PIPELINE_SOURCE CI_COMMIT_MESSAGE CI_COMMIT_TITLE diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index aee70bcfb29ba7eea067b0653a0c73a7df91f54f..e01906f4b6c44fe5e21bed6b4a0c21e5c1ea5cf4 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -20,6 +20,7 @@ describe Deployment do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:deployment) } + let(:scope) { :project } let(:scope_attrs) { { project: instance.project } } let(:usage) { :deployments } end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 128acf8368603d6e2fb9ea1f6f132c6f230314f8..e818fbeb9cfcfd4bee0b41f12a440314005aa600 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -17,6 +17,7 @@ describe Issue do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:issue) } + let(:scope) { :project } let(:scope_attrs) { { project: instance.project } } let(:usage) { :issues } end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 9ffa91fc265d86799f1d4f9a304e1968b552800d..65cc9372cbec9f4b6c6a20508f13aee85600ce43 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -84,6 +84,7 @@ describe MergeRequest do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:merge_request) } + let(:scope) { :target_project } let(:scope_attrs) { { project: instance.target_project } } let(:usage) { :merge_requests } end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 4bb9717d33eca3c7b4d5854739d02da8b12a2ee2..204d6b47832aca548f779c9a57e20cbc9aebab9a 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -6,6 +6,7 @@ describe Milestone do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:milestone, project: build(:project), group: nil) } + let(:scope) { :project } let(:scope_attrs) { { project: instance.project } } let(:usage) { :milestones } end @@ -15,6 +16,7 @@ describe Milestone do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:milestone, project: nil, group: build(:group)) } + let(:scope) { :group } let(:scope_attrs) { { namespace: instance.group } } let(:usage) { :milestones } end diff --git a/spec/support/shared_examples/models/atomic_internal_id_spec.rb b/spec/support/shared_examples/models/atomic_internal_id_spec.rb index 6a6e13418a987a8f1d4f2b03d958344c7d65688d..7ab1041d17ce09b97055083264b087ee6a91ef34 100644 --- a/spec/support/shared_examples/models/atomic_internal_id_spec.rb +++ b/spec/support/shared_examples/models/atomic_internal_id_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -shared_examples_for 'AtomicInternalId' do +shared_examples_for 'AtomicInternalId' do |validate_presence: true| describe '.has_internal_id' do describe 'Module inclusion' do subject { described_class } @@ -9,14 +9,31 @@ shared_examples_for 'AtomicInternalId' do end describe 'Validation' do - subject { instance } - before do - allow(InternalId).to receive(:generate_next).and_return(nil) + allow_any_instance_of(described_class).to receive(:"ensure_#{scope}_#{internal_id_attribute}!") + + instance.valid? end - it { is_expected.to validate_presence_of(internal_id_attribute) } - it { is_expected.to validate_numericality_of(internal_id_attribute) } + context 'when presence validation is required' do + before do + skip unless validate_presence + end + + it 'validates presence' do + expect(instance.errors[internal_id_attribute]).to include("can't be blank") + end + end + + context 'when presence validation is not required' do + before do + skip if validate_presence + end + + it 'does not validate presence' do + expect(instance.errors[internal_id_attribute]).to be_empty + end + end end describe 'Creating an instance' do