From e9924687147b1222fa2df3765a1e3c37662028a2 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 31 Jul 2017 21:59:04 +0900 Subject: ini --- app/models/ci/runner.rb | 2 +- app/views/projects/runners/_form.html.haml | 6 ++++++ app/views/projects/runners/show.html.haml | 3 +++ db/migrate/20170731123938_add_protected_to_ci_runners.rb | 15 +++++++++++++++ db/schema.rb | 1 + 5 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20170731123938_add_protected_to_ci_runners.rb diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 906a76ec560..05b45be5e91 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -5,7 +5,7 @@ module Ci RUNNER_QUEUE_EXPIRY_TIME = 60.minutes ONLINE_CONTACT_TIMEOUT = 1.hour AVAILABLE_SCOPES = %w[specific shared active paused online].freeze - FORM_EDITABLE = %i[description tag_list active run_untagged locked].freeze + FORM_EDITABLE = %i[description tag_list active run_untagged locked protected].freeze has_many :builds has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 2ef1f98ba48..7dae8ecbb94 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -6,6 +6,12 @@ .checkbox = f.check_box :active %span.light Paused Runners don't accept new jobs + .form-group + = label :protected, "Protected", class: 'control-label' + .col-sm-10 + .checkbox + = f.check_box :protected + %span.light This runner will only run on pipelines trigged on protected branches .form-group = label :run_untagged, 'Run untagged jobs', class: 'control-label' .col-sm-10 diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index 49415ba557b..e51bb299938 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -19,6 +19,9 @@ %tr %td Active %td= @runner.active? ? 'Yes' : 'No' + %tr + %td Protected + %td= @runner.protected? ? 'Yes' : 'No' %tr %td Can run untagged jobs %td= @runner.run_untagged? ? 'Yes' : 'No' diff --git a/db/migrate/20170731123938_add_protected_to_ci_runners.rb b/db/migrate/20170731123938_add_protected_to_ci_runners.rb new file mode 100644 index 00000000000..3782e047eee --- /dev/null +++ b/db/migrate/20170731123938_add_protected_to_ci_runners.rb @@ -0,0 +1,15 @@ +class AddProtectedToCiRunners < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:ci_runners, :protected, :boolean, default: false) + end + + def down + remove_column(:ci_runners, :protected) + end +end diff --git a/db/schema.rb b/db/schema.rb index 434d1326419..be12fc07e81 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -371,6 +371,7 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.string "architecture" t.boolean "run_untagged", default: true, null: false t.boolean "locked", default: false, null: false + t.boolean "protected", default: false, null: false end add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree -- cgit v1.2.1 From e5fd565c74b95815806d139f30676a895b88e29f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 31 Jul 2017 23:25:11 +0900 Subject: Solution 1. Persists protected(ref) flag on ci_pipelines table. builds_for_shared_runner and builds_for_specific_runner read the flag instead of executing protected_for? one by one. --- app/models/ci/build.rb | 10 ++++++++++ app/services/ci/create_pipeline_service.rb | 7 ++++++- app/services/ci/register_job_service.rb | 14 +++++++++----- .../20170731132235_add_protected_to_ci_pipelines.rb | 15 +++++++++++++++ db/schema.rb | 1 + 5 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20170731132235_add_protected_to_ci_pipelines.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8adaafe6439..709503f805c 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -35,6 +35,16 @@ module Ci scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } + scope :on_protected, ->() do + joins("LEFT JOIN ci_pipelines ON ci_builds.commit_id = ci_pipelines.id") + .where('ci_pipelines.protected IS TRUE') + end + + scope :unprotected, ->() do + joins("LEFT JOIN ci_pipelines ON ci_builds.commit_id = ci_pipelines.id") + .where('ci_pipelines.protected IS FALSE') + end + mount_uploader :artifacts_file, ArtifactUploader mount_uploader :artifacts_metadata, ArtifactUploader diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index de2cd7e87be..4b141047bce 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -12,7 +12,8 @@ module Ci tag: tag?, trigger_requests: Array(trigger_request), user: current_user, - pipeline_schedule: schedule + pipeline_schedule: schedule, + protected: protected? ) result = validate(current_user, @@ -175,6 +176,10 @@ module Ci origin_sha && origin_sha != Gitlab::Git::BLANK_SHA end + def protected? + project.protected_for?(ref) + end + def error(message, save: false) pipeline.tap do pipeline.errors.add(:base, message) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 414f672cc6a..93b1c86aad4 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -54,21 +54,25 @@ module Ci private def builds_for_shared_runner - new_builds. + b = new_builds. # don't run projects which have not enabled shared runners and builds joins(:project).where(projects: { shared_runners_enabled: true, pending_delete: false }) .joins('LEFT JOIN project_features ON ci_builds.project_id = project_features.project_id') - .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0'). + .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0') + + b = runner.protected? ? b.on_protected : b.unprotected # Implement fair scheduling # this returns builds that are ordered by number of running builds # we prefer projects that don't use shared runners at all - joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id=project_builds.project_id") - .order('COALESCE(project_builds.running_builds, 0) ASC', 'ci_builds.id ASC') + b.joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id=project_builds.project_id") + .order('COALESCE(project_builds.running_builds, 0) ASC', 'ci_builds.id ASC') end def builds_for_specific_runner - new_builds.where(project: runner.projects.without_deleted.with_builds_enabled).order('created_at ASC') + b = new_builds.where(project: runner.projects.without_deleted.with_builds_enabled) + b = runner.protected? ? b.on_protected : b.unprotected + b.order('created_at ASC') end def running_builds_for_shared_runners diff --git a/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb b/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb new file mode 100644 index 00000000000..bc034f99609 --- /dev/null +++ b/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb @@ -0,0 +1,15 @@ +class AddProtectedToCiPipelines < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:ci_pipelines, :protected, :boolean, default: false) + end + + def down + remove_column(:ci_pipelines, :protected) + end +end diff --git a/db/schema.rb b/db/schema.rb index be12fc07e81..df5755ff888 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -336,6 +336,7 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.integer "auto_canceled_by_id" t.integer "pipeline_schedule_id" t.integer "source" + t.boolean "protected", default: false, null: false end add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree -- cgit v1.2.1 From 0968e6ad4240eae8016340a550b4d871d823a903 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 1 Aug 2017 17:08:14 +0900 Subject: Tweak only new_builds. Revert the rest. --- app/models/ci/build.rb | 5 ----- app/services/ci/register_job_service.rb | 18 ++++++++---------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 709503f805c..8a285d55aaa 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -40,11 +40,6 @@ module Ci .where('ci_pipelines.protected IS TRUE') end - scope :unprotected, ->() do - joins("LEFT JOIN ci_pipelines ON ci_builds.commit_id = ci_pipelines.id") - .where('ci_pipelines.protected IS FALSE') - end - mount_uploader :artifacts_file, ArtifactUploader mount_uploader :artifacts_metadata, ArtifactUploader diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 93b1c86aad4..a51c28808dd 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -54,25 +54,21 @@ module Ci private def builds_for_shared_runner - b = new_builds. + new_builds. # don't run projects which have not enabled shared runners and builds joins(:project).where(projects: { shared_runners_enabled: true, pending_delete: false }) .joins('LEFT JOIN project_features ON ci_builds.project_id = project_features.project_id') - .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0') - - b = runner.protected? ? b.on_protected : b.unprotected + .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0'). # Implement fair scheduling # this returns builds that are ordered by number of running builds # we prefer projects that don't use shared runners at all - b.joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id=project_builds.project_id") - .order('COALESCE(project_builds.running_builds, 0) ASC', 'ci_builds.id ASC') + joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id=project_builds.project_id") + .order('COALESCE(project_builds.running_builds, 0) ASC', 'ci_builds.id ASC') end def builds_for_specific_runner - b = new_builds.where(project: runner.projects.without_deleted.with_builds_enabled) - b = runner.protected? ? b.on_protected : b.unprotected - b.order('created_at ASC') + new_builds.where(project: runner.projects.without_deleted.with_builds_enabled).order('created_at ASC') end def running_builds_for_shared_runners @@ -81,7 +77,9 @@ module Ci end def new_builds - Ci::Build.pending.unstarted + builds = Ci::Build.pending.unstarted + builds = builds.on_protected if runner.protected? + builds end def shared_runner_build_limits_feature_enabled? -- cgit v1.2.1 From 23ac401b6e9883d2bd6beaef30e686d67ece791e Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 1 Aug 2017 17:38:47 +0900 Subject: Allow null for protected attribute in ci_pipelines --- db/migrate/20170731132235_add_protected_to_ci_pipelines.rb | 2 +- db/schema.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb b/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb index bc034f99609..d2d2b6733f6 100644 --- a/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb +++ b/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb @@ -6,7 +6,7 @@ class AddProtectedToCiPipelines < ActiveRecord::Migration disable_ddl_transaction! def up - add_column_with_default(:ci_pipelines, :protected, :boolean, default: false) + add_column(:ci_pipelines, :protected, :boolean) end def down diff --git a/db/schema.rb b/db/schema.rb index df5755ff888..85cb4e99b10 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -336,7 +336,7 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.integer "auto_canceled_by_id" t.integer "pipeline_schedule_id" t.integer "source" - t.boolean "protected", default: false, null: false + t.boolean "protected" end add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree -- cgit v1.2.1 From 0a7b3ae9f1a67645c798dfbfc60388c4c9cb5b95 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 16 Aug 2017 23:13:43 +0900 Subject: Re-organize schema. Drop protected from runner. Add access_level to runner. Drop protected from pipeline. Add protected to ci_bilds. --- app/models/ci/runner.rb | 5 +++++ db/migrate/20170731123938_add_protected_to_ci_runners.rb | 15 --------------- .../20170731132235_add_protected_to_ci_pipelines.rb | 15 --------------- .../20170816133938_add_access_level_to_ci_runners.rb | 16 ++++++++++++++++ ...0170816133939_add_index_on_ci_runners_access_level.rb | 15 +++++++++++++++ db/migrate/20170816133940_add_protected_to_ci_builds.rb | 7 +++++++ db/schema.rb | 5 +++-- 7 files changed, 46 insertions(+), 32 deletions(-) delete mode 100644 db/migrate/20170731123938_add_protected_to_ci_runners.rb delete mode 100644 db/migrate/20170731132235_add_protected_to_ci_pipelines.rb create mode 100644 db/migrate/20170816133938_add_access_level_to_ci_runners.rb create mode 100644 db/migrate/20170816133939_add_index_on_ci_runners_access_level.rb create mode 100644 db/migrate/20170816133940_add_protected_to_ci_builds.rb diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 05b45be5e91..fd96592e6db 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -40,6 +40,11 @@ module Ci after_destroy :cleanup_runner_queue + enum access_level: { + protection_none: 0, + protection_full: 1 + } + # Searches for runners matching the given query. # # This method uses ILIKE on PostgreSQL and LIKE on MySQL. diff --git a/db/migrate/20170731123938_add_protected_to_ci_runners.rb b/db/migrate/20170731123938_add_protected_to_ci_runners.rb deleted file mode 100644 index 3782e047eee..00000000000 --- a/db/migrate/20170731123938_add_protected_to_ci_runners.rb +++ /dev/null @@ -1,15 +0,0 @@ -class AddProtectedToCiRunners < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_column_with_default(:ci_runners, :protected, :boolean, default: false) - end - - def down - remove_column(:ci_runners, :protected) - end -end diff --git a/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb b/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb deleted file mode 100644 index d2d2b6733f6..00000000000 --- a/db/migrate/20170731132235_add_protected_to_ci_pipelines.rb +++ /dev/null @@ -1,15 +0,0 @@ -class AddProtectedToCiPipelines < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_column(:ci_pipelines, :protected, :boolean) - end - - def down - remove_column(:ci_pipelines, :protected) - end -end diff --git a/db/migrate/20170816133938_add_access_level_to_ci_runners.rb b/db/migrate/20170816133938_add_access_level_to_ci_runners.rb new file mode 100644 index 00000000000..8cda0c3a717 --- /dev/null +++ b/db/migrate/20170816133938_add_access_level_to_ci_runners.rb @@ -0,0 +1,16 @@ +class AddAccessLevelToCiRunners < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + # Ci::Runner.protection_none: 0 + add_column_with_default(:ci_runners, :access_level, :integer, default: 0) + end + + def down + remove_column(:ci_runners, :access_level) + end +end diff --git a/db/migrate/20170816133939_add_index_on_ci_runners_access_level.rb b/db/migrate/20170816133939_add_index_on_ci_runners_access_level.rb new file mode 100644 index 00000000000..4239bf6f9cb --- /dev/null +++ b/db/migrate/20170816133939_add_index_on_ci_runners_access_level.rb @@ -0,0 +1,15 @@ +class AddIndexOnCiRunnersAccessLevel < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :ci_runners, :access_level + end + + def down + remove_concurrent_index :ci_runners, :access_level if index_exists?(:ci_runners, :access_level) + end +end diff --git a/db/migrate/20170816133940_add_protected_to_ci_builds.rb b/db/migrate/20170816133940_add_protected_to_ci_builds.rb new file mode 100644 index 00000000000..c73a4387d29 --- /dev/null +++ b/db/migrate/20170816133940_add_protected_to_ci_builds.rb @@ -0,0 +1,7 @@ +class AddProtectedToCiBuilds < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column :ci_builds, :protected, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 85cb4e99b10..c933b47ab01 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -246,6 +246,7 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.integer "auto_canceled_by_id" t.boolean "retried" t.integer "stage_id" + t.boolean "protected" end add_index "ci_builds", ["auto_canceled_by_id"], name: "index_ci_builds_on_auto_canceled_by_id", using: :btree @@ -336,7 +337,6 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.integer "auto_canceled_by_id" t.integer "pipeline_schedule_id" t.integer "source" - t.boolean "protected" end add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree @@ -372,9 +372,10 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.string "architecture" t.boolean "run_untagged", default: true, null: false t.boolean "locked", default: false, null: false - t.boolean "protected", default: false, null: false + t.integer "access_level", default: 0, null: false end + add_index "ci_runners", ["access_level"], name: "index_ci_runners_on_access_level", using: :btree add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree add_index "ci_runners", ["is_shared"], name: "index_ci_runners_on_is_shared", using: :btree add_index "ci_runners", ["locked"], name: "index_ci_runners_on_locked", using: :btree -- cgit v1.2.1 From e1ef436d1fd6419ce8a08c4ac33bf664786c80b0 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 17 Aug 2017 01:54:05 +0900 Subject: Update application code by the db schema change --- app/models/ci/build.rb | 6 +----- app/models/ci/runner.rb | 6 +++--- app/services/ci/create_pipeline_service.rb | 7 +------ app/services/ci/register_job_service.rb | 2 +- app/views/projects/runners/_form.html.haml | 2 +- app/views/projects/runners/show.html.haml | 2 +- db/migrate/20170816133938_add_access_level_to_ci_runners.rb | 2 +- lib/gitlab/ci/stage/seed.rb | 7 ++++++- 8 files changed, 15 insertions(+), 19 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8a285d55aaa..04d5b85ab01 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -34,11 +34,7 @@ module Ci scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } - - scope :on_protected, ->() do - joins("LEFT JOIN ci_pipelines ON ci_builds.commit_id = ci_pipelines.id") - .where('ci_pipelines.protected IS TRUE') - end + scope :protected_, -> { where(protected: true) } mount_uploader :artifacts_file, ArtifactUploader mount_uploader :artifacts_metadata, ArtifactUploader diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index fd96592e6db..24b62555845 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -5,7 +5,7 @@ module Ci RUNNER_QUEUE_EXPIRY_TIME = 60.minutes ONLINE_CONTACT_TIMEOUT = 1.hour AVAILABLE_SCOPES = %w[specific shared active paused online].freeze - FORM_EDITABLE = %i[description tag_list active run_untagged locked protected].freeze + FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level].freeze has_many :builds has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -41,8 +41,8 @@ module Ci after_destroy :cleanup_runner_queue enum access_level: { - protection_none: 0, - protection_full: 1 + unprotected: 0, + protected_: 1 } # Searches for runners matching the given query. diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 4b141047bce..de2cd7e87be 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -12,8 +12,7 @@ module Ci tag: tag?, trigger_requests: Array(trigger_request), user: current_user, - pipeline_schedule: schedule, - protected: protected? + pipeline_schedule: schedule ) result = validate(current_user, @@ -176,10 +175,6 @@ module Ci origin_sha && origin_sha != Gitlab::Git::BLANK_SHA end - def protected? - project.protected_for?(ref) - end - def error(message, save: false) pipeline.tap do pipeline.errors.add(:base, message) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index a51c28808dd..1f243de5761 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -78,7 +78,7 @@ module Ci def new_builds builds = Ci::Build.pending.unstarted - builds = builds.on_protected if runner.protected? + builds = builds.protected_ if runner.protected_? builds end diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 7dae8ecbb94..2942ce541ed 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -10,7 +10,7 @@ = label :protected, "Protected", class: 'control-label' .col-sm-10 .checkbox - = f.check_box :protected + = f.check_box :access_level, 1, 0 %span.light This runner will only run on pipelines trigged on protected branches .form-group = label :run_untagged, 'Run untagged jobs', class: 'control-label' diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index e51bb299938..9d427d18272 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -21,7 +21,7 @@ %td= @runner.active? ? 'Yes' : 'No' %tr %td Protected - %td= @runner.protected? ? 'Yes' : 'No' + %td= @runner.protected_? ? 'Yes' : 'No' %tr %td Can run untagged jobs %td= @runner.run_untagged? ? 'Yes' : 'No' diff --git a/db/migrate/20170816133938_add_access_level_to_ci_runners.rb b/db/migrate/20170816133938_add_access_level_to_ci_runners.rb index 8cda0c3a717..58317d46eac 100644 --- a/db/migrate/20170816133938_add_access_level_to_ci_runners.rb +++ b/db/migrate/20170816133938_add_access_level_to_ci_runners.rb @@ -6,7 +6,7 @@ class AddAccessLevelToCiRunners < ActiveRecord::Migration disable_ddl_transaction! def up - # Ci::Runner.protection_none: 0 + # Ci::Runner.unprotected: 0 add_column_with_default(:ci_runners, :access_level, :integer, default: 0) end diff --git a/lib/gitlab/ci/stage/seed.rb b/lib/gitlab/ci/stage/seed.rb index f81f9347b4d..d9ac46908b0 100644 --- a/lib/gitlab/ci/stage/seed.rb +++ b/lib/gitlab/ci/stage/seed.rb @@ -28,10 +28,15 @@ module Gitlab attributes.merge(project: project, ref: pipeline.ref, tag: pipeline.tag, - trigger_request: trigger) + trigger_request: trigger + protected: protected?) end end + def protected? + @protected ||= project.protected_for?(pipeline.ref) + end + def create! pipeline.stages.create!(stage).tap do |stage| builds_attributes = builds.map do |attributes| -- cgit v1.2.1 From eda34b1a1846a5d5b55cc127a32b0c7628580f25 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 21 Aug 2017 17:24:44 +0900 Subject: Add spec. Fix runner setting page. It worked. --- app/controllers/projects/runners_controller.rb | 4 +++- app/views/projects/runners/_form.html.haml | 2 +- ...0170816133938_add_access_level_to_ci_runners.rb | 4 ++-- lib/gitlab/ci/stage/seed.rb | 2 +- spec/factories/ci/runners.rb | 8 ++++++++ spec/models/ci/runner_spec.rb | 24 ++++++++++++++++++++++ 6 files changed, 39 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 9f9773575a5..bacecff3e7c 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -59,6 +59,8 @@ class Projects::RunnersController < Projects::ApplicationController end def runner_params - params.require(:runner).permit(Ci::Runner::FORM_EDITABLE) + params.require(:runner).permit(Ci::Runner::FORM_EDITABLE).tap do |params| + params['access_level'] = params['access_level']&.to_i + end end end diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 2942ce541ed..d66383bd8f1 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -10,7 +10,7 @@ = label :protected, "Protected", class: 'control-label' .col-sm-10 .checkbox - = f.check_box :access_level, 1, 0 + = f.check_box :access_level, {}, Ci::Runner.access_levels['protected_'], Ci::Runner.access_levels['unprotected'] %span.light This runner will only run on pipelines trigged on protected branches .form-group = label :run_untagged, 'Run untagged jobs', class: 'control-label' diff --git a/db/migrate/20170816133938_add_access_level_to_ci_runners.rb b/db/migrate/20170816133938_add_access_level_to_ci_runners.rb index 58317d46eac..cb8d023e439 100644 --- a/db/migrate/20170816133938_add_access_level_to_ci_runners.rb +++ b/db/migrate/20170816133938_add_access_level_to_ci_runners.rb @@ -6,8 +6,8 @@ class AddAccessLevelToCiRunners < ActiveRecord::Migration disable_ddl_transaction! def up - # Ci::Runner.unprotected: 0 - add_column_with_default(:ci_runners, :access_level, :integer, default: 0) + add_column_with_default(:ci_runners, :access_level, :integer, + default: Ci::Runner.access_levels['unprotected']) end def down diff --git a/lib/gitlab/ci/stage/seed.rb b/lib/gitlab/ci/stage/seed.rb index d9ac46908b0..2bc78b4f004 100644 --- a/lib/gitlab/ci/stage/seed.rb +++ b/lib/gitlab/ci/stage/seed.rb @@ -28,7 +28,7 @@ module Gitlab attributes.merge(project: project, ref: pipeline.ref, tag: pipeline.tag, - trigger_request: trigger + trigger_request: trigger, protected: protected?) end end diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index 05abf60d5ce..3a7da2e47d0 100644 --- a/spec/factories/ci/runners.rb +++ b/spec/factories/ci/runners.rb @@ -21,5 +21,13 @@ FactoryGirl.define do trait :inactive do active false end + + trait :protected do + access_level Ci::Runner.access_levels['protected_'] + end + + trait :unprotected do + access_level Ci::Runner.access_levels['unprotected'] + end end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 48f878bbee6..45a64b6f76f 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -456,4 +456,28 @@ describe Ci::Runner do expect(described_class.search(runner.description.upcase)).to eq([runner]) end end + + describe '.access_level' do + context 'when access_level of a runner is protected' do + before do + create(:ci_runner, :protected) + end + + it 'a protected runner exists' do + expect(Ci::Runner.count).to eq(1) + expect(Ci::Runner.last.protected_?).to eq(true) + end + end + + context 'when access_level of a runner is unprotected' do + before do + create(:ci_runner, :unprotected) + end + + it 'an unprotected runner exists' do + expect(Ci::Runner.count).to eq(1) + expect(Ci::Runner.last.unprotected?).to eq(true) + end + end + end end -- cgit v1.2.1 From bbe967abeba7be1db79e34439e74cd113c240b52 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 22 Aug 2017 17:01:11 +0900 Subject: Add the rest of specs --- lib/gitlab/ci/stage/seed.rb | 12 +++++---- spec/factories/ci/builds.rb | 8 ++++++ spec/lib/gitlab/ci/stage/seed_spec.rb | 11 ++++++++ spec/models/ci/build_spec.rb | 10 +++++++ spec/services/ci/register_job_service_spec.rb | 38 +++++++++++++++++++++++++++ 5 files changed, 74 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/ci/stage/seed.rb b/lib/gitlab/ci/stage/seed.rb index 2bc78b4f004..e19aae35a81 100644 --- a/lib/gitlab/ci/stage/seed.rb +++ b/lib/gitlab/ci/stage/seed.rb @@ -29,14 +29,10 @@ module Gitlab ref: pipeline.ref, tag: pipeline.tag, trigger_request: trigger, - protected: protected?) + protected: protected_ref?) end end - def protected? - @protected ||= project.protected_for?(pipeline.ref) - end - def create! pipeline.stages.create!(stage).tap do |stage| builds_attributes = builds.map do |attributes| @@ -48,6 +44,12 @@ module Gitlab end end end + + private + + def protected_ref? + @protected_ref ||= project.protected_for?(pipeline.ref) + end end end end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 5bba1dec7db..f8922275860 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -226,5 +226,13 @@ FactoryGirl.define do status 'created' self.when 'manual' end + + trait(:protected) do + protected true + end + + trait(:unprotected) do + protected false + end end end diff --git a/spec/lib/gitlab/ci/stage/seed_spec.rb b/spec/lib/gitlab/ci/stage/seed_spec.rb index d7e91a5a62c..1e9cbbdfb77 100644 --- a/spec/lib/gitlab/ci/stage/seed_spec.rb +++ b/spec/lib/gitlab/ci/stage/seed_spec.rb @@ -26,6 +26,17 @@ describe Gitlab::Ci::Stage::Seed do expect(subject.builds).to all(include(project: pipeline.project)) expect(subject.builds) .to all(include(trigger_request: pipeline.trigger_requests.first)) + expect(subject.builds).to all(include(protected: true)) + end + + context 'when a ref is unprotected' do + before do + allow_any_instance_of(Project).to receive(:protected_for?).and_return(false) + end + + it 'returns unprotected builds' do + expect(subject.builds).to all(include(protected: false)) + end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 0c35ad3c9d8..1673d873b57 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -43,6 +43,16 @@ describe Ci::Build do it { is_expected.not_to include(manual_but_created) } end + describe '.protected_' do + let!(:protected_job) { create(:ci_build, :protected) } + let!(:unprotected_job) { create(:ci_build, :unprotected) } + + subject { described_class.protected_ } + + it { is_expected.to include(protected_job) } + it { is_expected.not_to include(unprotected_job) } + end + describe '#actionize' do context 'when build is a created' do before do diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 8eb0d2d10a4..99f2507e59c 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -215,6 +215,44 @@ module Ci end end + context 'when a runner is unprotected' do + context 'when a job is protected' do + let!(:pending_build) { create(:ci_build, :protected, pipeline: pipeline) } + + it 'picks the protected job' do + expect(execute(specific_runner)).to eq(pending_build) + end + end + + context 'when a job is unprotected' do + let!(:pending_build) { create(:ci_build, :unprotected, pipeline: pipeline) } + + it 'picks the unprotected job' do + expect(execute(specific_runner)).to eq(pending_build) + end + end + end + + context 'when a runner is protected' do + let!(:specific_runner) { create(:ci_runner, :protected, :specific) } + + context 'when a job is protected' do + let!(:pending_build) { create(:ci_build, :protected, pipeline: pipeline) } + + it 'picks the protected job' do + expect(execute(specific_runner)).to eq(pending_build) + end + end + + context 'when a job is unprotected' do + let!(:unprotected_job) { create(:ci_build, :unprotected, pipeline: pipeline) } + + it 'does not pick the unprotected job' do + expect(execute(specific_runner)).to be_nil + end + end + end + def execute(runner) described_class.new(runner).execute.build end -- cgit v1.2.1 From 1925bfd2a5b8fab48de8b1f2a4ae7db6a09736e1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 22 Aug 2017 22:59:08 +0900 Subject: Fix spec --- app/controllers/projects/runners_controller.rb | 2 +- spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 + spec/models/ci/runner_spec.rb | 8 ++++---- spec/services/ci/retry_build_service_spec.rb | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index bacecff3e7c..d07eddbd7fd 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -60,7 +60,7 @@ class Projects::RunnersController < Projects::ApplicationController def runner_params params.require(:runner).permit(Ci::Runner::FORM_EDITABLE).tap do |params| - params['access_level'] = params['access_level']&.to_i + params['access_level'] = params['access_level'].to_i if params['access_level'] 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 a5e03e149a7..8c4810f292e 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -276,6 +276,7 @@ CommitStatus: - coverage_regex - auto_canceled_by_id - retried +- protected Ci::Variable: - id - project_id diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 45a64b6f76f..a04d4615b6b 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -464,8 +464,8 @@ describe Ci::Runner do end it 'a protected runner exists' do - expect(Ci::Runner.count).to eq(1) - expect(Ci::Runner.last.protected_?).to eq(true) + expect(described_class.count).to eq(1) + expect(described_class.last.protected_?).to eq(true) end end @@ -475,8 +475,8 @@ describe Ci::Runner do end it 'an unprotected runner exists' do - expect(Ci::Runner.count).to eq(1) - expect(Ci::Runner.last.unprotected?).to eq(true) + expect(described_class.count).to eq(1) + expect(described_class.last.unprotected?).to eq(true) end end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index cec667071cc..d32ed9c33ee 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -22,7 +22,7 @@ describe Ci::RetryBuildService do %i[type lock_version target_url base_tags commit_id deployments erased_by_id last_deployment project_id runner_id tag_taggings taggings tags trigger_request_id - user_id auto_canceled_by_id retried].freeze + user_id auto_canceled_by_id retried protected].freeze shared_examples 'build duplication' do let(:stage) do -- cgit v1.2.1 From 665774b4775f9e7ae3adfccf1c36bf9867fa53a5 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 23 Aug 2017 22:02:09 +0900 Subject: Add changelog --- ...e-sm-33281-protected-runner-executes-jobs-on-protected-branch.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-sm-33281-protected-runner-executes-jobs-on-protected-branch.yml diff --git a/changelogs/unreleased/feature-sm-33281-protected-runner-executes-jobs-on-protected-branch.yml b/changelogs/unreleased/feature-sm-33281-protected-runner-executes-jobs-on-protected-branch.yml new file mode 100644 index 00000000000..b57b9a3dfbe --- /dev/null +++ b/changelogs/unreleased/feature-sm-33281-protected-runner-executes-jobs-on-protected-branch.yml @@ -0,0 +1,5 @@ +--- +title: Protected runners +merge_request: 13194 +author: +type: added -- cgit v1.2.1 From af112c55e54874a37e780de6723c64e9be61b6e8 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 24 Aug 2017 01:07:35 +0900 Subject: Add doc --- doc/ci/runners/README.md | 19 +++++++++++++++++++ doc/ci/runners/img/protected_runners_check_box.png | Bin 0 -> 8584 bytes doc/ci/runners/img/specific_runners_edit_icon.png | Bin 0 -> 1414 bytes 3 files changed, 19 insertions(+) create mode 100644 doc/ci/runners/img/protected_runners_check_box.png create mode 100644 doc/ci/runners/img/specific_runners_edit_icon.png diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index 76d746155eb..e5750ee6896 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -107,6 +107,23 @@ To lock/unlock a Runner: 1. Check the **Lock to current projects** option 1. Click **Save changes** for the changes to take effect +## Protected runners + +>**Notes:** +This feature requires GitLab 10.0 or higher. + +You can protect runners from revealing sensitive information. +Whenever a runner is protected, the runner picks only jobs created on +[protected branches] or [protected tags], and ignore other jobs. + +To protect/unprotect runners: + +1. Visit your project's **Settings ➔ Pipelines** +1. Find a runner you want to protect/unprotect and make sure it's enabled +1. Click a pencil button ![specific runners edit icon](img/specific_runners_edit_icon.png) resides runner name +1. Check the **Protected** option +1. Click **Save changes** for the changes to take effect + ## How shared Runners pick jobs Shared Runners abide to a process queue we call fair usage. The fair usage @@ -218,3 +235,5 @@ We're always looking for contributions that can mitigate these [install]: http://docs.gitlab.com/runner/install/ [fifo]: https://en.wikipedia.org/wiki/FIFO_(computing_and_electronics) [register]: http://docs.gitlab.com/runner/register/ +[protected branches]: ../../user/project/protected_branches.md +[protected tags]: ../../user/project/protected_tags.md diff --git a/doc/ci/runners/img/protected_runners_check_box.png b/doc/ci/runners/img/protected_runners_check_box.png new file mode 100644 index 00000000000..fb58498c7ce Binary files /dev/null and b/doc/ci/runners/img/protected_runners_check_box.png differ diff --git a/doc/ci/runners/img/specific_runners_edit_icon.png b/doc/ci/runners/img/specific_runners_edit_icon.png new file mode 100644 index 00000000000..e83cf6e3858 Binary files /dev/null and b/doc/ci/runners/img/specific_runners_edit_icon.png differ -- cgit v1.2.1 From 6f19fc1147a60f279db35428993ac532841195ad Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 24 Aug 2017 01:28:57 +0900 Subject: Add API support --- doc/api/runners.md | 9 +++++++-- lib/api/entities.rb | 1 + lib/api/runners.rb | 3 ++- spec/requests/api/runners_spec.rb | 4 +++- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/doc/api/runners.md b/doc/api/runners.md index 16d362a3530..dcca30dbfa7 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -138,7 +138,8 @@ Example response: "ruby", "mysql" ], - "version": null + "version": null, + "access_level": 0 } ``` @@ -156,6 +157,9 @@ PUT /runners/:id | `description` | string | no | The description of a runner | | `active` | boolean | no | The state of a runner; can be set to `true` or `false` | | `tag_list` | array | no | The list of tags for a runner; put array of tags, that should be finally assigned to a runner | +| `run_untagged` | boolean | no | Flag indicating the runner can execute untagged jobs | +| `locked` | boolean | no | Flag indicating the runner is locked | +| `access_level` | integer | no | The access_level of the runner; `unprotected`: 0, `protected`: 1 | ``` curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/runners/6" --form "description=test-1-20150125-test" --form "tag_list=ruby,mysql,tag1,tag2" @@ -190,7 +194,8 @@ Example response: "tag1", "tag2" ], - "version": null + "version": null, + "access_level": 0 } ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 9df9a515990..f13f2d723bb 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -775,6 +775,7 @@ module API expose :tag_list expose :run_untagged expose :locked + expose :access_level expose :version, :revision, :platform, :architecture expose :contacted_at expose :token, if: lambda { |runner, options| options[:current_user].admin? || !runner.is_shared? } diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 1ea9a7918d7..d8fc44e5790 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -55,7 +55,8 @@ module API optional :tag_list, type: Array[String], desc: 'The list of tags for a runner' optional :run_untagged, type: Boolean, desc: 'Flag indicating the runner can execute untagged jobs' optional :locked, type: Boolean, desc: 'Flag indicating the runner is locked' - at_least_one_of :description, :active, :tag_list, :run_untagged, :locked + optional :access_level, type: Integer, desc: 'The access_level of the runner' + at_least_one_of :description, :active, :tag_list, :run_untagged, :locked, :access_level end put ':id' do runner = get_runner(params.delete(:id)) diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 244895a417e..b8d98818a8c 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -191,7 +191,8 @@ describe API::Runners do active: !active, tag_list: ['ruby2.1', 'pgsql', 'mysql'], run_untagged: 'false', - locked: 'true') + locked: 'true', + access_level: 1) shared_runner.reload expect(response).to have_http_status(200) @@ -200,6 +201,7 @@ describe API::Runners do expect(shared_runner.tag_list).to include('ruby2.1', 'pgsql', 'mysql') expect(shared_runner.run_untagged?).to be(false) expect(shared_runner.locked?).to be(true) + expect(shared_runner.protected_?).to be_truthy expect(shared_runner.ensure_runner_queue_value) .not_to eq(runner_queue_value) end -- cgit v1.2.1 From 641491777032482632e63e0d968a0bad32856794 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 24 Aug 2017 01:59:01 +0900 Subject: Remove conversion string to interger for access_level --- app/controllers/projects/runners_controller.rb | 4 +--- app/views/projects/runners/_form.html.haml | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index d07eddbd7fd..9f9773575a5 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -59,8 +59,6 @@ class Projects::RunnersController < Projects::ApplicationController end def runner_params - params.require(:runner).permit(Ci::Runner::FORM_EDITABLE).tap do |params| - params['access_level'] = params['access_level'].to_i if params['access_level'] - end + params.require(:runner).permit(Ci::Runner::FORM_EDITABLE) end end diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index d66383bd8f1..8c98075eeb5 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -10,7 +10,7 @@ = label :protected, "Protected", class: 'control-label' .col-sm-10 .checkbox - = f.check_box :access_level, {}, Ci::Runner.access_levels['protected_'], Ci::Runner.access_levels['unprotected'] + = f.check_box :access_level, {}, 'protected_', 'unprotected' %span.light This runner will only run on pipelines trigged on protected branches .form-group = label :run_untagged, 'Run untagged jobs', class: 'control-label' -- cgit v1.2.1 From 84626fbb6c6deca47534e7e3d5a843d03494c7d3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 25 Aug 2017 17:46:38 +0900 Subject: Add feature spec --- spec/features/runners_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index 785cfeb34bd..c7f0e342809 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -43,6 +43,21 @@ feature 'Runners' do expect(page).not_to have_content(specific_runner.display_name) end + scenario 'user edits the runner to be protected' do + visit runners_path(project) + + within '.activated-specific-runners' do + first('.edit-runner > a').click + end + + expect(page.find_field('runner[access_level]')).not_to be_checked + + check 'runner_access_level' + click_button 'Save changes' + + expect(page).to have_content 'Protected Yes' + end + context 'when a runner has a tag' do background do specific_runner.update(tag_list: ['tag']) -- cgit v1.2.1 From 1f114df25ac3f5336316b09dc3ef65da03373318 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 25 Aug 2017 17:48:33 +0900 Subject: Fix doc --- doc/ci/runners/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index e5750ee6896..01198744c19 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -124,6 +124,8 @@ To protect/unprotect runners: 1. Check the **Protected** option 1. Click **Save changes** for the changes to take effect +![specific runners edit icon](img/protected_runners_check_box.png) + ## How shared Runners pick jobs Shared Runners abide to a process queue we call fair usage. The fair usage -- cgit v1.2.1 From 6fbb3ce6e93b8dfba795d4542fcb5602c4c82eaf Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 28 Aug 2017 18:19:45 +0900 Subject: Fix doc --- doc/ci/runners/README.md | 19 ++++++++++--------- doc/ci/runners/img/specific_runners_edit_icon.png | Bin 1414 -> 0 bytes 2 files changed, 10 insertions(+), 9 deletions(-) delete mode 100644 doc/ci/runners/img/specific_runners_edit_icon.png diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index 01198744c19..4ccf1b56771 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -107,24 +107,25 @@ To lock/unlock a Runner: 1. Check the **Lock to current projects** option 1. Click **Save changes** for the changes to take effect -## Protected runners +## Protected Runners >**Notes:** -This feature requires GitLab 10.0 or higher. +[Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13194) +in GitLab 10.0. -You can protect runners from revealing sensitive information. -Whenever a runner is protected, the runner picks only jobs created on -[protected branches] or [protected tags], and ignore other jobs. +You can protect Runners from revealing sensitive information. +Whenever a Runner is protected, the Runner picks only jobs created on +[protected branches] or [protected tags], and ignores other jobs. -To protect/unprotect runners: +To protect/unprotect Runners: 1. Visit your project's **Settings ➔ Pipelines** -1. Find a runner you want to protect/unprotect and make sure it's enabled -1. Click a pencil button ![specific runners edit icon](img/specific_runners_edit_icon.png) resides runner name +1. Find a Runner you want to protect/unprotect and make sure it's enabled +1. Click the pencil button besides the Runner name 1. Check the **Protected** option 1. Click **Save changes** for the changes to take effect -![specific runners edit icon](img/protected_runners_check_box.png) +![specific Runners edit icon](img/protected_runners_check_box.png) ## How shared Runners pick jobs diff --git a/doc/ci/runners/img/specific_runners_edit_icon.png b/doc/ci/runners/img/specific_runners_edit_icon.png deleted file mode 100644 index e83cf6e3858..00000000000 Binary files a/doc/ci/runners/img/specific_runners_edit_icon.png and /dev/null differ -- cgit v1.2.1 From 4b0e31ba64118011ffc29c31bc771fa2568cd270 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 28 Aug 2017 21:10:01 +0900 Subject: Extend can_pick? --- app/models/ci/runner.rb | 2 ++ spec/models/ci/runner_spec.rb | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 24b62555845..a3b4a1fccf3 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -111,6 +111,8 @@ module Ci end def can_pick?(build) + return false if self.protected_? && !build.protected? + assignable_for?(build.project) && accepting_tags?(build) end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index a04d4615b6b..566b9b48879 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -95,6 +95,8 @@ describe Ci::Runner do let(:build) { create(:ci_build, pipeline: pipeline) } let(:runner) { create(:ci_runner) } + subject { runner.can_pick?(build) } + before do build.project.runners << runner end @@ -222,6 +224,28 @@ describe Ci::Runner do end end end + + context 'when runner is protected' do + before do + runner.protected_! + end + + context 'when build is protected' do + before do + build.protected = true + end + + it { is_expected.to be_truthy } + end + + context 'when build is unprotected' do + before do + build.protected = false + end + + it { is_expected.to be_falsey } + end + end end describe '#status' do -- cgit v1.2.1 From 3875983205fd3cdbdc93f18b118deaf098d75af1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 28 Aug 2017 21:40:52 +0900 Subject: Impprove spec by godfat suggestions --- spec/factories/ci/runners.rb | 4 ++-- spec/lib/gitlab/ci/stage/seed_spec.rb | 11 ++++++++++- spec/requests/api/runners_spec.rb | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index 3a7da2e47d0..e563a14ce08 100644 --- a/spec/factories/ci/runners.rb +++ b/spec/factories/ci/runners.rb @@ -23,11 +23,11 @@ FactoryGirl.define do end trait :protected do - access_level Ci::Runner.access_levels['protected_'] + access_level 'protected_' end trait :unprotected do - access_level Ci::Runner.access_levels['unprotected'] + access_level 'unprotected' end end end diff --git a/spec/lib/gitlab/ci/stage/seed_spec.rb b/spec/lib/gitlab/ci/stage/seed_spec.rb index 1e9cbbdfb77..515e06e38e5 100644 --- a/spec/lib/gitlab/ci/stage/seed_spec.rb +++ b/spec/lib/gitlab/ci/stage/seed_spec.rb @@ -26,7 +26,16 @@ describe Gitlab::Ci::Stage::Seed do expect(subject.builds).to all(include(project: pipeline.project)) expect(subject.builds) .to all(include(trigger_request: pipeline.trigger_requests.first)) - expect(subject.builds).to all(include(protected: true)) + end + + context 'when a ref is protected' do + before do + allow_any_instance_of(Project).to receive(:protected_for?).and_return(true) + end + + it 'returns unprotected builds' do + expect(subject.builds).to all(include(protected: true)) + end end context 'when a ref is unprotected' do diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index b8d98818a8c..5a6edad8118 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -192,7 +192,7 @@ describe API::Runners do tag_list: ['ruby2.1', 'pgsql', 'mysql'], run_untagged: 'false', locked: 'true', - access_level: 1) + access_level: Ci::Runner.access_levels['protected_']) shared_runner.reload expect(response).to have_http_status(200) -- cgit v1.2.1 From 1024718e9fddbb0d61d3f64f44303964641fcdd8 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 29 Aug 2017 15:56:03 +0900 Subject: Refactor access_level to not_protected and ref_protected --- app/models/ci/build.rb | 2 +- app/models/ci/runner.rb | 6 +++--- app/services/ci/register_job_service.rb | 2 +- app/views/projects/runners/_form.html.haml | 2 +- app/views/projects/runners/show.html.haml | 2 +- db/migrate/20170816133938_add_access_level_to_ci_runners.rb | 2 +- doc/api/runners.md | 2 +- spec/factories/ci/runners.rb | 8 ++++---- spec/models/ci/build_spec.rb | 4 ++-- spec/models/ci/runner_spec.rb | 12 ++++++------ spec/requests/api/runners_spec.rb | 4 ++-- spec/services/ci/register_job_service_spec.rb | 2 +- 12 files changed, 24 insertions(+), 24 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 04d5b85ab01..e58484878ba 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -34,7 +34,7 @@ module Ci scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } - scope :protected_, -> { where(protected: true) } + scope :ref_protected, -> { where(protected: true) } mount_uploader :artifacts_file, ArtifactUploader mount_uploader :artifacts_metadata, ArtifactUploader diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index a3b4a1fccf3..f202f08b151 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -41,8 +41,8 @@ module Ci after_destroy :cleanup_runner_queue enum access_level: { - unprotected: 0, - protected_: 1 + not_protected: 0, + ref_protected: 1 } # Searches for runners matching the given query. @@ -111,7 +111,7 @@ module Ci end def can_pick?(build) - return false if self.protected_? && !build.protected? + return false if self.ref_protected? && !build.protected? assignable_for?(build.project) && accepting_tags?(build) end diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 1f243de5761..b8db709211a 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -78,7 +78,7 @@ module Ci def new_builds builds = Ci::Build.pending.unstarted - builds = builds.protected_ if runner.protected_? + builds = builds.ref_protected if runner.ref_protected? builds end diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 8c98075eeb5..ac8e15a48b2 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -10,7 +10,7 @@ = label :protected, "Protected", class: 'control-label' .col-sm-10 .checkbox - = f.check_box :access_level, {}, 'protected_', 'unprotected' + = f.check_box :access_level, {}, 'ref_protected', 'not_protected' %span.light This runner will only run on pipelines trigged on protected branches .form-group = label :run_untagged, 'Run untagged jobs', class: 'control-label' diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index 9d427d18272..dfab04aa1fb 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -21,7 +21,7 @@ %td= @runner.active? ? 'Yes' : 'No' %tr %td Protected - %td= @runner.protected_? ? 'Yes' : 'No' + %td= @runner.ref_protected? ? 'Yes' : 'No' %tr %td Can run untagged jobs %td= @runner.run_untagged? ? 'Yes' : 'No' diff --git a/db/migrate/20170816133938_add_access_level_to_ci_runners.rb b/db/migrate/20170816133938_add_access_level_to_ci_runners.rb index cb8d023e439..fc484730f42 100644 --- a/db/migrate/20170816133938_add_access_level_to_ci_runners.rb +++ b/db/migrate/20170816133938_add_access_level_to_ci_runners.rb @@ -7,7 +7,7 @@ class AddAccessLevelToCiRunners < ActiveRecord::Migration def up add_column_with_default(:ci_runners, :access_level, :integer, - default: Ci::Runner.access_levels['unprotected']) + default: Ci::Runner.access_levels['not_protected']) end def down diff --git a/doc/api/runners.md b/doc/api/runners.md index dcca30dbfa7..df458af77bb 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -159,7 +159,7 @@ PUT /runners/:id | `tag_list` | array | no | The list of tags for a runner; put array of tags, that should be finally assigned to a runner | | `run_untagged` | boolean | no | Flag indicating the runner can execute untagged jobs | | `locked` | boolean | no | Flag indicating the runner is locked | -| `access_level` | integer | no | The access_level of the runner; `unprotected`: 0, `protected`: 1 | +| `access_level` | integer | no | The access_level of the runner; `not_protected`: 0, `ref_protected`: 1 | ``` curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/runners/6" --form "description=test-1-20150125-test" --form "tag_list=ruby,mysql,tag1,tag2" diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index e563a14ce08..249d8bde5a1 100644 --- a/spec/factories/ci/runners.rb +++ b/spec/factories/ci/runners.rb @@ -22,12 +22,12 @@ FactoryGirl.define do active false end - trait :protected do - access_level 'protected_' + trait :ref_protected do + access_level 'ref_protected' end - trait :unprotected do - access_level 'unprotected' + trait :not_protected do + access_level 'not_protected' end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 1673d873b57..ef6c91d11f1 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -43,11 +43,11 @@ describe Ci::Build do it { is_expected.not_to include(manual_but_created) } end - describe '.protected_' do + describe '.ref_protected' do let!(:protected_job) { create(:ci_build, :protected) } let!(:unprotected_job) { create(:ci_build, :unprotected) } - subject { described_class.protected_ } + subject { described_class.ref_protected } it { is_expected.to include(protected_job) } it { is_expected.not_to include(unprotected_job) } diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 566b9b48879..54f68542d41 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -227,7 +227,7 @@ describe Ci::Runner do context 'when runner is protected' do before do - runner.protected_! + runner.ref_protected! end context 'when build is protected' do @@ -489,18 +489,18 @@ describe Ci::Runner do it 'a protected runner exists' do expect(described_class.count).to eq(1) - expect(described_class.last.protected_?).to eq(true) + expect(described_class.last.ref_protected?).to eq(true) end end - context 'when access_level of a runner is unprotected' do + context 'when access_level of a runner is not_protected' do before do - create(:ci_runner, :unprotected) + create(:ci_runner, :not_protected) end - it 'an unprotected runner exists' do + it 'an not_protected runner exists' do expect(described_class.count).to eq(1) - expect(described_class.last.unprotected?).to eq(true) + expect(described_class.last.not_protected?).to eq(true) end end end diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 5a6edad8118..abaa6eb4f6d 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -192,7 +192,7 @@ describe API::Runners do tag_list: ['ruby2.1', 'pgsql', 'mysql'], run_untagged: 'false', locked: 'true', - access_level: Ci::Runner.access_levels['protected_']) + access_level: Ci::Runner.access_levels['ref_protected']) shared_runner.reload expect(response).to have_http_status(200) @@ -201,7 +201,7 @@ describe API::Runners do expect(shared_runner.tag_list).to include('ruby2.1', 'pgsql', 'mysql') expect(shared_runner.run_untagged?).to be(false) expect(shared_runner.locked?).to be(true) - expect(shared_runner.protected_?).to be_truthy + expect(shared_runner.ref_protected?).to be_truthy expect(shared_runner.ensure_runner_queue_value) .not_to eq(runner_queue_value) end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 99f2507e59c..eb3e7e749ef 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -215,7 +215,7 @@ module Ci end end - context 'when a runner is unprotected' do + context 'when a runner is not_protected' do context 'when a job is protected' do let!(:pending_build) { create(:ci_build, :protected, pipeline: pipeline) } -- cgit v1.2.1 From 13b9b5f11a556b2841aabbf46516d1acab79aa0d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 29 Aug 2017 16:09:30 +0900 Subject: Improve API arguments as String --- doc/api/runners.md | 6 +++--- lib/api/runners.rb | 3 ++- spec/requests/api/runners_spec.rb | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/doc/api/runners.md b/doc/api/runners.md index df458af77bb..8146a7e0647 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -139,7 +139,7 @@ Example response: "mysql" ], "version": null, - "access_level": 0 + "access_level": "ref_protected" } ``` @@ -159,7 +159,7 @@ PUT /runners/:id | `tag_list` | array | no | The list of tags for a runner; put array of tags, that should be finally assigned to a runner | | `run_untagged` | boolean | no | Flag indicating the runner can execute untagged jobs | | `locked` | boolean | no | Flag indicating the runner is locked | -| `access_level` | integer | no | The access_level of the runner; `not_protected`: 0, `ref_protected`: 1 | +| `access_level` | integer | no | The access_level of the runner; `not_protected` or `ref_protected` | ``` curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/runners/6" --form "description=test-1-20150125-test" --form "tag_list=ruby,mysql,tag1,tag2" @@ -195,7 +195,7 @@ Example response: "tag2" ], "version": null, - "access_level": 0 + "access_level": "ref_protected" } ``` diff --git a/lib/api/runners.rb b/lib/api/runners.rb index d8fc44e5790..d3559ef71be 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -55,7 +55,8 @@ module API optional :tag_list, type: Array[String], desc: 'The list of tags for a runner' optional :run_untagged, type: Boolean, desc: 'Flag indicating the runner can execute untagged jobs' optional :locked, type: Boolean, desc: 'Flag indicating the runner is locked' - optional :access_level, type: Integer, desc: 'The access_level of the runner' + optional :access_level, type: String, values: Ci::Runner.access_levels.keys, + desc: 'The access_level of the runner' at_least_one_of :description, :active, :tag_list, :run_untagged, :locked, :access_level end put ':id' do diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index abaa6eb4f6d..67907579225 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -192,7 +192,7 @@ describe API::Runners do tag_list: ['ruby2.1', 'pgsql', 'mysql'], run_untagged: 'false', locked: 'true', - access_level: Ci::Runner.access_levels['ref_protected']) + access_level: 'ref_protected') shared_runner.reload expect(response).to have_http_status(200) -- cgit v1.2.1 From 2170eec662e05b24148dc1c959dbe5fe9723b4fb Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 29 Aug 2017 17:31:59 +0900 Subject: Fix spec --- spec/models/ci/runner_spec.rb | 4 ++-- spec/services/ci/register_job_service_spec.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 54f68542d41..8c157d4369b 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -482,9 +482,9 @@ describe Ci::Runner do end describe '.access_level' do - context 'when access_level of a runner is protected' do + context 'when access_level of a runner is ref_protected' do before do - create(:ci_runner, :protected) + create(:ci_runner, :ref_protected) end it 'a protected runner exists' do diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index eb3e7e749ef..e9bcb41c5f5 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -233,8 +233,8 @@ module Ci end end - context 'when a runner is protected' do - let!(:specific_runner) { create(:ci_runner, :protected, :specific) } + context 'when a runner is ref_protected' do + let!(:specific_runner) { create(:ci_runner, :ref_protected, :specific) } context 'when a job is protected' do let!(:pending_build) { create(:ci_build, :protected, pipeline: pipeline) } -- cgit v1.2.1 From a2cde2847c68e8061f6f40f106502fa164be7b02 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 29 Aug 2017 18:57:53 +0900 Subject: Fix runner api doc --- doc/api/runners.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/runners.md b/doc/api/runners.md index 8146a7e0647..6304a496f94 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -159,7 +159,7 @@ PUT /runners/:id | `tag_list` | array | no | The list of tags for a runner; put array of tags, that should be finally assigned to a runner | | `run_untagged` | boolean | no | Flag indicating the runner can execute untagged jobs | | `locked` | boolean | no | Flag indicating the runner is locked | -| `access_level` | integer | no | The access_level of the runner; `not_protected` or `ref_protected` | +| `access_level` | string | no | The access_level of the runner; `not_protected` or `ref_protected` | ``` curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/runners/6" --form "description=test-1-20150125-test" --form "tag_list=ruby,mysql,tag1,tag2" -- cgit v1.2.1 From d3bf01608013eac24764c1dacd51f8b841dd9bf1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 31 Aug 2017 17:01:43 +0900 Subject: Remove access_level index from runner. Add protected on ci_pipelines. Add protected index on ci_builds. --- app/services/ci/create_pipeline_service.rb | 3 ++- ...20170816133939_add_index_on_ci_runners_access_level.rb | 15 --------------- .../20170816143940_add_protected_to_ci_pipelines.rb | 7 +++++++ .../20170816153940_add_index_on_ci_builds_protected.rb | 15 +++++++++++++++ db/schema.rb | 5 +++-- spec/services/ci/create_pipeline_service_spec.rb | 2 ++ 6 files changed, 29 insertions(+), 18 deletions(-) delete mode 100644 db/migrate/20170816133939_add_index_on_ci_runners_access_level.rb create mode 100644 db/migrate/20170816143940_add_protected_to_ci_pipelines.rb create mode 100644 db/migrate/20170816153940_add_index_on_ci_builds_protected.rb diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index de2cd7e87be..414c01b2546 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -12,7 +12,8 @@ module Ci tag: tag?, trigger_requests: Array(trigger_request), user: current_user, - pipeline_schedule: schedule + pipeline_schedule: schedule, + protected: project.protected_for?(ref) ) result = validate(current_user, diff --git a/db/migrate/20170816133939_add_index_on_ci_runners_access_level.rb b/db/migrate/20170816133939_add_index_on_ci_runners_access_level.rb deleted file mode 100644 index 4239bf6f9cb..00000000000 --- a/db/migrate/20170816133939_add_index_on_ci_runners_access_level.rb +++ /dev/null @@ -1,15 +0,0 @@ -class AddIndexOnCiRunnersAccessLevel < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_concurrent_index :ci_runners, :access_level - end - - def down - remove_concurrent_index :ci_runners, :access_level if index_exists?(:ci_runners, :access_level) - end -end diff --git a/db/migrate/20170816143940_add_protected_to_ci_pipelines.rb b/db/migrate/20170816143940_add_protected_to_ci_pipelines.rb new file mode 100644 index 00000000000..ce8f1e03686 --- /dev/null +++ b/db/migrate/20170816143940_add_protected_to_ci_pipelines.rb @@ -0,0 +1,7 @@ +class AddProtectedToCiPipelines < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column :ci_pipelines, :protected, :boolean + end +end diff --git a/db/migrate/20170816153940_add_index_on_ci_builds_protected.rb b/db/migrate/20170816153940_add_index_on_ci_builds_protected.rb new file mode 100644 index 00000000000..caf7c705a6e --- /dev/null +++ b/db/migrate/20170816153940_add_index_on_ci_builds_protected.rb @@ -0,0 +1,15 @@ +class AddIndexOnCiBuildsProtected < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :ci_builds, :protected + end + + def down + remove_concurrent_index :ci_builds, :protected if index_exists?(:ci_builds, :protected) + end +end diff --git a/db/schema.rb b/db/schema.rb index c933b47ab01..c41c9e98a4f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170824162758) do +ActiveRecord::Schema.define(version: 20170830125940) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -255,6 +255,7 @@ ActiveRecord::Schema.define(version: 20170824162758) do add_index "ci_builds", ["commit_id", "type", "name", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_name_and_ref", using: :btree add_index "ci_builds", ["commit_id", "type", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_ref", using: :btree add_index "ci_builds", ["project_id"], name: "index_ci_builds_on_project_id", using: :btree + add_index "ci_builds", ["protected"], name: "index_ci_builds_on_protected", using: :btree add_index "ci_builds", ["runner_id"], name: "index_ci_builds_on_runner_id", using: :btree add_index "ci_builds", ["stage_id"], name: "index_ci_builds_on_stage_id", using: :btree add_index "ci_builds", ["status", "type", "runner_id"], name: "index_ci_builds_on_status_and_type_and_runner_id", using: :btree @@ -337,6 +338,7 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.integer "auto_canceled_by_id" t.integer "pipeline_schedule_id" t.integer "source" + t.boolean "protected" end add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree @@ -375,7 +377,6 @@ ActiveRecord::Schema.define(version: 20170824162758) do t.integer "access_level", default: 0, null: false end - add_index "ci_runners", ["access_level"], name: "index_ci_runners_on_access_level", using: :btree add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree add_index "ci_runners", ["is_shared"], name: "index_ci_runners_on_is_shared", using: :btree add_index "ci_runners", ["locked"], name: "index_ci_runners_on_locked", using: :btree diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 4ba3dada37c..8b0573db846 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -398,6 +398,7 @@ describe Ci::CreatePipelineService do it 'creates a pipeline' do expect(execute_service).to be_persisted expect(Ci::Pipeline.count).to eq(1) + expect(Ci::Pipeline.last).to be_protected end end @@ -473,6 +474,7 @@ describe Ci::CreatePipelineService do expect(execute_service(trigger_request: trigger_request)) .to be_persisted expect(Ci::Pipeline.count).to eq(1) + expect(Ci::Pipeline.last).not_to be_protected end end end -- cgit v1.2.1 From 07f7a01b4da02533c417fa636f004fedf3c4c661 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 31 Aug 2017 18:28:29 +0900 Subject: Improve spec. Add validation for accel_level on runner. --- app/models/ci/runner.rb | 1 + spec/factories/ci/runners.rb | 4 +- spec/models/ci/build_spec.rb | 22 ++++++++--- spec/models/ci/runner_spec.rb | 54 +++++++++++++++------------ spec/services/ci/register_job_service_spec.rb | 6 ++- 5 files changed, 54 insertions(+), 33 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index f202f08b151..b1798084787 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -35,6 +35,7 @@ module Ci end validate :tag_constraints + validates :access_level, presence: true acts_as_taggable diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index 249d8bde5a1..efd9b34fb3f 100644 --- a/spec/factories/ci/runners.rb +++ b/spec/factories/ci/runners.rb @@ -23,11 +23,11 @@ FactoryGirl.define do end trait :ref_protected do - access_level 'ref_protected' + access_level :ref_protected end trait :not_protected do - access_level 'not_protected' + access_level :not_protected end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index ef6c91d11f1..a94fdbf8434 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -44,13 +44,25 @@ describe Ci::Build do end describe '.ref_protected' do - let!(:protected_job) { create(:ci_build, :protected) } - let!(:unprotected_job) { create(:ci_build, :unprotected) } - subject { described_class.ref_protected } - it { is_expected.to include(protected_job) } - it { is_expected.not_to include(unprotected_job) } + context 'when protected is true' do + let!(:job) { create(:ci_build, :protected) } + + it { is_expected.to include(job) } + end + + context 'when protected is false' do + let!(:job) { create(:ci_build, :unprotected) } + + it { is_expected.not_to include(job) } + end + + context 'when protected is false' do + let!(:job) { create(:ci_build, protected: nil) } + + it { is_expected.not_to include(job) } + end end describe '#actionize' do diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 8c157d4369b..ca48372c5e2 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' describe Ci::Runner do describe 'validation' do + it { is_expected.to validate_presence_of(:access_level) } + context 'when runner is not allowed to pick untagged jobs' do context 'when runner does not have tags' do it 'is not valid' do @@ -19,6 +21,34 @@ describe Ci::Runner do end end + describe '#access_level' do + context 'when creating new runner and access_level is nil' do + let(:runner) do + build(:ci_runner, access_level: nil) + end + + it "object is invalid" do + expect(runner).not_to be_valid + end + end + + context 'when creating new runner and access_level is defined in enum' do + let(:runner) do + build(:ci_runner, access_level: :not_protected) + end + + it "object is valid" do + expect(runner).to be_valid + end + end + + context 'when creating new runner and access_level is not defined in enum' do + it "raises an error" do + expect { build(:ci_runner, access_level: :this_is_not_defined) }.to raise_error(ArgumentError) + end + end + end + describe '#display_name' do it 'returns the description if it has a value' do runner = FactoryGirl.build(:ci_runner, description: 'Linux/Ruby-1.9.3-p448') @@ -480,28 +510,4 @@ describe Ci::Runner do expect(described_class.search(runner.description.upcase)).to eq([runner]) end end - - describe '.access_level' do - context 'when access_level of a runner is ref_protected' do - before do - create(:ci_runner, :ref_protected) - end - - it 'a protected runner exists' do - expect(described_class.count).to eq(1) - expect(described_class.last.ref_protected?).to eq(true) - end - end - - context 'when access_level of a runner is not_protected' do - before do - create(:ci_runner, :not_protected) - end - - it 'an not_protected runner exists' do - expect(described_class.count).to eq(1) - expect(described_class.last.not_protected?).to eq(true) - end - end - end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index e9bcb41c5f5..6f8ebf5d3ba 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -215,7 +215,9 @@ module Ci end end - context 'when a runner is not_protected' do + context 'when access_level of runner is not_protected' do + let!(:specific_runner) { create(:ci_runner, :not_protected, :specific) } + context 'when a job is protected' do let!(:pending_build) { create(:ci_build, :protected, pipeline: pipeline) } @@ -233,7 +235,7 @@ module Ci end end - context 'when a runner is ref_protected' do + context 'when access_level of runner is ref_protected' do let!(:specific_runner) { create(:ci_runner, :ref_protected, :specific) } context 'when a job is protected' do -- cgit v1.2.1 From f3d3cecf5a7691c5df48fc6b0f3d206cd57203db Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 31 Aug 2017 20:25:25 +0900 Subject: Fix schema. Add safe_model_attributes for pipelines --- db/schema.rb | 2 +- spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index c41c9e98a4f..a5f867df9ae 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170830125940) do +ActiveRecord::Schema.define(version: 20170824162758) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 8c4810f292e..27f2ce60084 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -224,6 +224,7 @@ Ci::Pipeline: - lock_version - auto_canceled_by_id - pipeline_schedule_id +- protected Ci::Stage: - id - name -- cgit v1.2.1 From ce7c0ac3dbdc3f2f2f34b39bf5ef3755e79e9d42 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 31 Aug 2017 21:13:40 +0900 Subject: Add validation for protected attributes --- app/models/ci/build.rb | 1 + app/models/ci/pipeline.rb | 1 + app/services/ci/retry_build_service.rb | 2 +- spec/factories/ci/builds.rb | 5 +++-- spec/factories/ci/pipelines.rb | 9 +++++++++ spec/models/ci/build_spec.rb | 6 +++++- 6 files changed, 20 insertions(+), 4 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index e58484878ba..11717a1bb12 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -26,6 +26,7 @@ module Ci validates :coverage, numericality: true, allow_blank: true validates :ref, presence: true + validates :protected, inclusion: { in: [ true, false ] }, on: :create scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 2d40f8012a3..d877e51c2a7 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -36,6 +36,7 @@ module Ci validates :sha, presence: { unless: :importing? } validates :ref, presence: { unless: :importing? } validates :status, presence: { unless: :importing? } + validates :protected, inclusion: { in: [ true, false ], unless: :importing? }, on: :create validate :valid_commit_sha, unless: :importing? after_create :keep_around_commits, unless: :importing? diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index ea3b8d66ed9..d67b9f5cc56 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -3,7 +3,7 @@ module Ci CLONE_ACCESSORS = %i[pipeline project ref tag options commands name allow_failure stage_id stage stage_idx trigger_request yaml_variables when environment coverage_regex - description tag_list].freeze + description tag_list protected].freeze def execute(build) reprocess!(build).tap do |new_build| diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index f8922275860..bdc3e8acc07 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -12,6 +12,7 @@ FactoryGirl.define do started_at 'Di 29. Okt 09:51:28 CET 2013' finished_at 'Di 29. Okt 09:53:28 CET 2013' commands 'ls -a' + protected false options do { @@ -227,11 +228,11 @@ FactoryGirl.define do self.when 'manual' end - trait(:protected) do + trait :protected do protected true end - trait(:unprotected) do + trait :unprotected do protected false end end diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb index e83a0e599a8..5b51f5898a3 100644 --- a/spec/factories/ci/pipelines.rb +++ b/spec/factories/ci/pipelines.rb @@ -4,6 +4,7 @@ FactoryGirl.define do ref 'master' sha '97de212e80737a608d939f648d959671fb0a0142' status 'pending' + protected false project @@ -59,6 +60,14 @@ FactoryGirl.define do trait :failed do status :failed end + + trait :protected do + protected true + end + + trait :unprotected do + protected false + end end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index a94fdbf8434..cab59db3580 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -59,7 +59,11 @@ describe Ci::Build do end context 'when protected is false' do - let!(:job) { create(:ci_build, protected: nil) } + let!(:job) { create(:ci_build) } + + before do + job.update_attribute(:protected, nil) + end it { is_expected.not_to include(job) } end -- cgit v1.2.1 From 5547baa3eb1979e8de36f00174c1f98ddc3dabd0 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 1 Sep 2017 01:51:55 +0900 Subject: Fix spec --- app/models/ci/build.rb | 2 +- app/models/ci/pipeline.rb | 2 +- spec/support/cycle_analytics_helpers.rb | 6 ++++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 11717a1bb12..78fcfff3ea0 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -26,7 +26,7 @@ module Ci validates :coverage, numericality: true, allow_blank: true validates :ref, presence: true - validates :protected, inclusion: { in: [ true, false ] }, on: :create + validates :protected, inclusion: { in: [true, false] }, on: :create scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index d877e51c2a7..ca9a350ea79 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -36,7 +36,7 @@ module Ci validates :sha, presence: { unless: :importing? } validates :ref, presence: { unless: :importing? } validates :status, presence: { unless: :importing? } - validates :protected, inclusion: { in: [ true, false ], unless: :importing? }, on: :create + validates :protected, inclusion: { in: [true, false], unless: :importing? }, on: :create validate :valid_commit_sha, unless: :importing? after_create :keep_around_commits, unless: :importing? diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb index 39586d37e93..e6b66c4baed 100644 --- a/spec/support/cycle_analytics_helpers.rb +++ b/spec/support/cycle_analytics_helpers.rb @@ -80,7 +80,8 @@ module CycleAnalyticsHelpers sha: project.repository.commit('master').sha, ref: 'master', source: :push, - project: project) + project: project, + protected: true) end def new_dummy_job(environment) @@ -93,7 +94,8 @@ module CycleAnalyticsHelpers ref: 'master', tag: false, name: 'dummy', - pipeline: dummy_pipeline) + pipeline: dummy_pipeline, + protected: true) end end -- cgit v1.2.1 From d8478d166b96b7b40b27b162d6afd170a5c8d763 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 1 Sep 2017 17:54:07 +0900 Subject: Fix spec --- app/models/ci/build.rb | 3 ++- features/steps/project/pages.rb | 3 ++- lib/api/commit_statuses.rb | 6 ++++-- spec/requests/api/commit_statuses_spec.rb | 4 ++-- spec/requests/api/commits_spec.rb | 2 +- spec/requests/api/v3/commits_spec.rb | 4 ++-- spec/services/ci/retry_build_service_spec.rb | 4 ++-- 7 files changed, 15 insertions(+), 11 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 78fcfff3ea0..ba3156154ac 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -3,6 +3,7 @@ module Ci include TokenAuthenticatable include AfterCommitQueue include Presentable + include Importable belongs_to :runner belongs_to :trigger_request @@ -26,7 +27,7 @@ module Ci validates :coverage, numericality: true, allow_blank: true validates :ref, presence: true - validates :protected, inclusion: { in: [true, false] }, on: :create + validates :protected, inclusion: { in: [true, false], unless: :importing? }, on: :create scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } diff --git a/features/steps/project/pages.rb b/features/steps/project/pages.rb index 9705470738e..124a132d688 100644 --- a/features/steps/project/pages.rb +++ b/features/steps/project/pages.rb @@ -37,7 +37,8 @@ class Spinach::Features::ProjectPages < Spinach::FeatureSteps step 'pages are deployed' do pipeline = @project.pipelines.create(ref: 'HEAD', sha: @project.commit('HEAD').sha, - source: :push) + source: :push, + protected: false) build = build(:ci_build, project: @project, diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 78e889a4c35..6314ea63197 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -74,7 +74,8 @@ module API source: :external, sha: commit.sha, ref: ref, - user: current_user) + user: current_user, + protected: @project.protected_for?(ref)) end status = GenericCommitStatus.running_or_pending.find_or_initialize_by( @@ -82,7 +83,8 @@ module API pipeline: pipeline, name: name, ref: ref, - user: current_user + user: current_user, + protected: @project.protected_for?(ref) ) optional_attributes = diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index 3c02e6302b4..cc71865e1f3 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -16,8 +16,8 @@ describe API::CommitStatuses do let(:get_url) { "/projects/#{project.id}/repository/commits/#{sha}/statuses" } context 'ci commit exists' do - let!(:master) { project.pipelines.create(source: :push, sha: commit.id, ref: 'master') } - let!(:develop) { project.pipelines.create(source: :push, sha: commit.id, ref: 'develop') } + let!(:master) { project.pipelines.create(source: :push, sha: commit.id, ref: 'master', protected: false) } + let!(:develop) { project.pipelines.create(source: :push, sha: commit.id, ref: 'develop', protected: false) } context "reporter user" do let(:statuses_id) { json_response.map { |status| status['id'] } } diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index d3b48f948f6..edbfaf510c5 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -565,7 +565,7 @@ describe API::Commits do end context 'when the ref has a pipeline' do - let!(:pipeline) { project.pipelines.create(source: :push, ref: 'master', sha: commit.sha) } + let!(:pipeline) { project.pipelines.create(source: :push, ref: 'master', sha: commit.sha, protected: false) } it 'includes a "created" status' do get api(route, current_user) diff --git a/spec/requests/api/v3/commits_spec.rb b/spec/requests/api/v3/commits_spec.rb index 8fb96b3c7c5..6d0ca33a6fa 100644 --- a/spec/requests/api/v3/commits_spec.rb +++ b/spec/requests/api/v3/commits_spec.rb @@ -386,7 +386,7 @@ describe API::V3::Commits do end it "returns status for CI" do - pipeline = project.pipelines.create(source: :push, ref: 'master', sha: project.repository.commit.sha) + pipeline = project.pipelines.create(source: :push, ref: 'master', sha: project.repository.commit.sha, protected: false) pipeline.update(status: 'success') get v3_api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user) @@ -396,7 +396,7 @@ describe API::V3::Commits do end it "returns status for CI when pipeline is created" do - project.pipelines.create(source: :push, ref: 'master', sha: project.repository.commit.sha) + project.pipelines.create(source: :push, ref: 'master', sha: project.repository.commit.sha, protected: false) get v3_api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user) diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index d32ed9c33ee..7c9c117bf71 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -22,7 +22,7 @@ describe Ci::RetryBuildService do %i[type lock_version target_url base_tags commit_id deployments erased_by_id last_deployment project_id runner_id tag_taggings taggings tags trigger_request_id - user_id auto_canceled_by_id retried protected].freeze + user_id auto_canceled_by_id retried].freeze shared_examples 'build duplication' do let(:stage) do @@ -48,7 +48,7 @@ describe Ci::RetryBuildService do describe 'clone accessors' do CLONE_ACCESSORS.each do |attribute| it "clones #{attribute} build attribute" do - expect(new_build.send(attribute)).to be_present + expect(new_build.send(attribute)).not_to be_nil expect(new_build.send(attribute)).to eq build.send(attribute) end end -- cgit v1.2.1 From eab938d50519a4da272818c1364ceeca82e32982 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 1 Sep 2017 20:30:42 +0900 Subject: Fix typo --- spec/lib/gitlab/ci/stage/seed_spec.rb | 2 +- spec/models/ci/build_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/lib/gitlab/ci/stage/seed_spec.rb b/spec/lib/gitlab/ci/stage/seed_spec.rb index 515e06e38e5..9ecd128faca 100644 --- a/spec/lib/gitlab/ci/stage/seed_spec.rb +++ b/spec/lib/gitlab/ci/stage/seed_spec.rb @@ -33,7 +33,7 @@ describe Gitlab::Ci::Stage::Seed do allow_any_instance_of(Project).to receive(:protected_for?).and_return(true) end - it 'returns unprotected builds' do + it 'returns protected builds' do expect(subject.builds).to all(include(protected: true)) end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index cab59db3580..1f965cfbede 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -58,7 +58,7 @@ describe Ci::Build do it { is_expected.not_to include(job) } end - context 'when protected is false' do + context 'when protected is nil' do let!(:job) { create(:ci_build) } before do -- cgit v1.2.1 From 1b481342a08caea34e0d605780de13d47d8dd7e2 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 2 Sep 2017 16:31:14 +0900 Subject: Fix spec --- spec/factories/ci/builds.rb | 4 --- spec/factories/ci/pipelines.rb | 4 --- spec/factories/ci/runners.rb | 5 +--- spec/models/ci/build_spec.rb | 2 +- spec/models/ci/runner_spec.rb | 24 ++++++++++++++- spec/services/ci/create_pipeline_service_spec.rb | 16 +++++----- spec/services/ci/register_job_service_spec.rb | 38 +++++++++++++++++++----- spec/support/cycle_analytics_helpers.rb | 4 +-- 8 files changed, 67 insertions(+), 30 deletions(-) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index bdc3e8acc07..25ec63de94a 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -231,9 +231,5 @@ FactoryGirl.define do trait :protected do protected true end - - trait :unprotected do - protected false - end end end diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb index 5b51f5898a3..e5ea6b41ea3 100644 --- a/spec/factories/ci/pipelines.rb +++ b/spec/factories/ci/pipelines.rb @@ -64,10 +64,6 @@ FactoryGirl.define do trait :protected do protected true end - - trait :unprotected do - protected false - end end end end diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index efd9b34fb3f..88bb755d068 100644 --- a/spec/factories/ci/runners.rb +++ b/spec/factories/ci/runners.rb @@ -5,6 +5,7 @@ FactoryGirl.define do platform "darwin" is_shared false active true + access_level :not_protected trait :online do contacted_at Time.now @@ -25,9 +26,5 @@ FactoryGirl.define do trait :ref_protected do access_level :ref_protected end - - trait :not_protected do - access_level :not_protected - end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 1f965cfbede..3fe3ec17d36 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -53,7 +53,7 @@ describe Ci::Build do end context 'when protected is false' do - let!(:job) { create(:ci_build, :unprotected) } + let!(:job) { create(:ci_build) } it { is_expected.not_to include(job) } end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index ca48372c5e2..2e686e515c5 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -255,7 +255,29 @@ describe Ci::Runner do end end - context 'when runner is protected' do + context 'when access_level of runner is not_protected' do + before do + runner.not_protected! + end + + context 'when build is protected' do + before do + build.protected = true + end + + it { is_expected.to be_truthy } + end + + context 'when build is unprotected' do + before do + build.protected = false + end + + it { is_expected.to be_truthy } + end + end + + context 'when access_level of runner is ref_protected' do before do runner.ref_protected! end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 8b0573db846..49d7c663128 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -391,14 +391,16 @@ describe Ci::CreatePipelineService do end context 'when user is master' do + let(:pipeline) { execute_service } + before do project.add_master(user) end - it 'creates a pipeline' do - expect(execute_service).to be_persisted + it 'creates a protected pipeline' do + expect(pipeline).to be_persisted + expect(pipeline).to be_protected expect(Ci::Pipeline.count).to eq(1) - expect(Ci::Pipeline.last).to be_protected end end @@ -469,12 +471,12 @@ describe Ci::CreatePipelineService do let(:user) {} let(:trigger) { create(:ci_trigger, owner: nil) } let(:trigger_request) { create(:ci_trigger_request, trigger: trigger) } + let(:pipeline) { execute_service(trigger_request: trigger_request) } - it 'creates a pipeline' do - expect(execute_service(trigger_request: trigger_request)) - .to be_persisted + it 'creates an unprotected pipeline' do + expect(pipeline).to be_persisted + expect(pipeline).not_to be_protected expect(Ci::Pipeline.count).to eq(1) - expect(Ci::Pipeline.last).not_to be_protected end end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 6f8ebf5d3ba..6bc3dadbc71 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -219,18 +219,30 @@ module Ci let!(:specific_runner) { create(:ci_runner, :not_protected, :specific) } context 'when a job is protected' do - let!(:pending_build) { create(:ci_build, :protected, pipeline: pipeline) } + let!(:protected_job) { create(:ci_build, :protected, pipeline: pipeline) } it 'picks the protected job' do - expect(execute(specific_runner)).to eq(pending_build) + expect(execute(specific_runner)).to eq(protected_job) end end context 'when a job is unprotected' do - let!(:pending_build) { create(:ci_build, :unprotected, pipeline: pipeline) } + let!(:unprotected_job) { create(:ci_build, pipeline: pipeline) } it 'picks the unprotected job' do - expect(execute(specific_runner)).to eq(pending_build) + expect(execute(specific_runner)).to eq(unprotected_job) + end + end + + context 'when protected attribute of a job is nil' do + let!(:legacy_job) { create(:ci_build, pipeline: pipeline) } + + before do + legacy_job.update_attribute(:protected, nil) + end + + it 'picks the legacy job' do + expect(execute(specific_runner)).to eq(legacy_job) end end end @@ -239,20 +251,32 @@ module Ci let!(:specific_runner) { create(:ci_runner, :ref_protected, :specific) } context 'when a job is protected' do - let!(:pending_build) { create(:ci_build, :protected, pipeline: pipeline) } + let!(:protected_job) { create(:ci_build, :protected, pipeline: pipeline) } it 'picks the protected job' do - expect(execute(specific_runner)).to eq(pending_build) + expect(execute(specific_runner)).to eq(protected_job) end end context 'when a job is unprotected' do - let!(:unprotected_job) { create(:ci_build, :unprotected, pipeline: pipeline) } + let!(:unprotected_job) { create(:ci_build, pipeline: pipeline) } it 'does not pick the unprotected job' do expect(execute(specific_runner)).to be_nil end end + + context 'when protected attribute of a job is nil' do + let!(:legacy_job) { create(:ci_build, pipeline: pipeline) } + + before do + legacy_job.update_attribute(:protected, nil) + end + + it 'does not pick the legacy job' do + expect(execute(specific_runner)).to be_nil + end + end end def execute(runner) diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb index e6b66c4baed..934b4557ba2 100644 --- a/spec/support/cycle_analytics_helpers.rb +++ b/spec/support/cycle_analytics_helpers.rb @@ -81,7 +81,7 @@ module CycleAnalyticsHelpers ref: 'master', source: :push, project: project, - protected: true) + protected: false) end def new_dummy_job(environment) @@ -95,7 +95,7 @@ module CycleAnalyticsHelpers tag: false, name: 'dummy', pipeline: dummy_pipeline, - protected: true) + protected: false) end end -- cgit v1.2.1 From 50b9aeb495eb12aba4c833956bad517ba2b515fe Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 2 Sep 2017 17:53:54 +0900 Subject: Fix spec --- spec/services/ci/register_job_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 6bc3dadbc71..4e20b5ca1da 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -216,7 +216,7 @@ module Ci end context 'when access_level of runner is not_protected' do - let!(:specific_runner) { create(:ci_runner, :not_protected, :specific) } + let!(:specific_runner) { create(:ci_runner, :specific) } context 'when a job is protected' do let!(:protected_job) { create(:ci_build, :protected, pipeline: pipeline) } -- cgit v1.2.1 From 53b5346d407d2303e88d8cf5d6e9271996051cf1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sun, 3 Sep 2017 23:58:25 +0900 Subject: Fix spec on egister_job_service_spec.rb --- spec/services/ci/register_job_service_spec.rb | 64 +++++++++++++-------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 4e20b5ca1da..5ac30111ec9 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -4,7 +4,7 @@ module Ci describe RegisterJobService do let!(:project) { FactoryGirl.create :project, shared_runners_enabled: false } let!(:pipeline) { FactoryGirl.create :ci_pipeline, project: project } - let!(:pending_build) { FactoryGirl.create :ci_build, pipeline: pipeline } + let!(:pending_job) { FactoryGirl.create :ci_build, pipeline: pipeline } let!(:shared_runner) { FactoryGirl.create(:ci_runner, is_shared: true) } let!(:specific_runner) { FactoryGirl.create(:ci_runner, is_shared: false) } @@ -15,32 +15,32 @@ module Ci describe '#execute' do context 'runner follow tag list' do it "picks build with the same tag" do - pending_build.tag_list = ["linux"] - pending_build.save + pending_job.tag_list = ["linux"] + pending_job.save specific_runner.tag_list = ["linux"] - expect(execute(specific_runner)).to eq(pending_build) + expect(execute(specific_runner)).to eq(pending_job) end it "does not pick build with different tag" do - pending_build.tag_list = ["linux"] - pending_build.save + pending_job.tag_list = ["linux"] + pending_job.save specific_runner.tag_list = ["win32"] expect(execute(specific_runner)).to be_falsey end it "picks build without tag" do - expect(execute(specific_runner)).to eq(pending_build) + expect(execute(specific_runner)).to eq(pending_job) end it "does not pick build with tag" do - pending_build.tag_list = ["linux"] - pending_build.save + pending_job.tag_list = ["linux"] + pending_job.save expect(execute(specific_runner)).to be_falsey end it "pick build without tag" do specific_runner.tag_list = ["win32"] - expect(execute(specific_runner)).to eq(pending_build) + expect(execute(specific_runner)).to eq(pending_job) end end @@ -76,7 +76,7 @@ module Ci let!(:pipeline2) { create :ci_pipeline, project: project2 } let!(:project3) { create :project, shared_runners_enabled: true } let!(:pipeline3) { create :ci_pipeline, project: project3 } - let!(:build1_project1) { pending_build } + let!(:build1_project1) { pending_job } let!(:build2_project1) { FactoryGirl.create :ci_build, pipeline: pipeline } let!(:build3_project1) { FactoryGirl.create :ci_build, pipeline: pipeline } let!(:build1_project2) { FactoryGirl.create :ci_build, pipeline: pipeline2 } @@ -172,7 +172,7 @@ module Ci context 'when first build is stalled' do before do - pending_build.lock_version = 10 + pending_job.lock_version = 10 end subject { described_class.new(specific_runner).execute } @@ -182,7 +182,7 @@ module Ci before do allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner) - .and_return([pending_build, other_build]) + .and_return([pending_job, other_build]) end it "receives second build from the queue" do @@ -194,7 +194,7 @@ module Ci context 'when single build is in queue' do before do allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner) - .and_return([pending_build]) + .and_return([pending_job]) end it "does not receive any valid result" do @@ -219,30 +219,30 @@ module Ci let!(:specific_runner) { create(:ci_runner, :specific) } context 'when a job is protected' do - let!(:protected_job) { create(:ci_build, :protected, pipeline: pipeline) } + let!(:pending_job) { create(:ci_build, :protected, pipeline: pipeline) } - it 'picks the protected job' do - expect(execute(specific_runner)).to eq(protected_job) + it 'picks the job' do + expect(execute(specific_runner)).to eq(pending_job) end end context 'when a job is unprotected' do - let!(:unprotected_job) { create(:ci_build, pipeline: pipeline) } + let!(:pending_job) { create(:ci_build, pipeline: pipeline) } - it 'picks the unprotected job' do - expect(execute(specific_runner)).to eq(unprotected_job) + it 'picks the job' do + expect(execute(specific_runner)).to eq(pending_job) end end context 'when protected attribute of a job is nil' do - let!(:legacy_job) { create(:ci_build, pipeline: pipeline) } + let!(:pending_job) { create(:ci_build, pipeline: pipeline) } before do - legacy_job.update_attribute(:protected, nil) + pending_job.update_attribute(:protected, nil) end - it 'picks the legacy job' do - expect(execute(specific_runner)).to eq(legacy_job) + it 'picks the job' do + expect(execute(specific_runner)).to eq(pending_job) end end end @@ -251,29 +251,29 @@ module Ci let!(:specific_runner) { create(:ci_runner, :ref_protected, :specific) } context 'when a job is protected' do - let!(:protected_job) { create(:ci_build, :protected, pipeline: pipeline) } + let!(:pending_job) { create(:ci_build, :protected, pipeline: pipeline) } - it 'picks the protected job' do - expect(execute(specific_runner)).to eq(protected_job) + it 'picks the job' do + expect(execute(specific_runner)).to eq(pending_job) end end context 'when a job is unprotected' do - let!(:unprotected_job) { create(:ci_build, pipeline: pipeline) } + let!(:pending_job) { create(:ci_build, pipeline: pipeline) } - it 'does not pick the unprotected job' do + it 'does not pick the job' do expect(execute(specific_runner)).to be_nil end end context 'when protected attribute of a job is nil' do - let!(:legacy_job) { create(:ci_build, pipeline: pipeline) } + let!(:pending_job) { create(:ci_build, pipeline: pipeline) } before do - legacy_job.update_attribute(:protected, nil) + pending_job.update_attribute(:protected, nil) end - it 'does not pick the legacy job' do + it 'does not pick the job' do expect(execute(specific_runner)).to be_nil end end -- cgit v1.2.1