From 2548c155eed13c806951a6303c85dbb0c5772ca3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 7 Apr 2017 04:02:19 +0900 Subject: [PATCH 01/19] Add form for scheduled trigger --- .../projects/settings/ci_cd_controller.rb | 1 + .../projects/triggers_controller.rb | 10 ++++++-- app/models/ci/trigger.rb | 13 +++++++++++ app/models/ci/trigger_schedule.rb | 8 ++++++- app/views/projects/triggers/_form.html.haml | 23 ++++++++++++++++++- app/views/projects/triggers/_index.html.haml | 2 ++ .../projects/triggers/_trigger.html.haml | 6 +++++ 7 files changed, 59 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 6f009d61950..0ae4b4d3945 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -27,6 +27,7 @@ module Projects def define_triggers_variables @triggers = @project.triggers @trigger = Ci::Trigger.new + @trigger.build_trigger_schedule end def define_badges_variables diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index c47198c5eb6..5ceeeb1c454 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -68,10 +68,16 @@ class Projects::TriggersController < Projects::ApplicationController end def create_params - params.require(:trigger).permit(:description) + params.require(:trigger).permit( + :description, :ref, :trigger_schedule_on, + trigger_schedule_attributes: [ :cron, :cron_timezone ] + ) end def update_params - params.require(:trigger).permit(:description) + params.require(:trigger).permit( + :description, :ref, :trigger_schedule_on, + trigger_schedule_attributes: [ :cron, :cron_timezone ] + ) end end diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 0a89f3e0640..225b6253616 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -14,8 +14,21 @@ module Ci before_validation :set_default_values + accepts_nested_attributes_for :trigger_schedule + + attr_accessor :trigger_schedule_on + def set_default_values self.token = SecureRandom.hex(15) if self.token.blank? + + if trigger_schedule_on.present? + if trigger_schedule_on.to_i == 1 + self.trigger_schedule.project = project + self.trigger_schedule.trigger = self + else + self.trigger_schedule = nil + end + end end def last_trigger_request diff --git a/app/models/ci/trigger_schedule.rb b/app/models/ci/trigger_schedule.rb index 10ea381ee31..d8a2770ea61 100644 --- a/app/models/ci/trigger_schedule.rb +++ b/app/models/ci/trigger_schedule.rb @@ -8,7 +8,7 @@ module Ci belongs_to :project belongs_to :trigger - delegate :ref, to: :trigger + delegate :ref, to: :trigger, allow_nil: true validates :trigger, presence: { unless: :importing? } validates :cron, cron: true, presence: { unless: :importing? } @@ -26,5 +26,11 @@ module Ci rescue ActiveRecord::RecordInvalid update_attribute(:next_run_at, nil) # update without validation end + + def real_next_run(worker_cron: Settings.cron_jobs['trigger_schedule_worker']['cron'], + worker_time_zone: Time.zone.name) + Gitlab::Ci::CronParser.new(worker_cron, worker_time_zone) + .next_time_from(next_run_at) + end end end diff --git a/app/views/projects/triggers/_form.html.haml b/app/views/projects/triggers/_form.html.haml index 5f708b3a2ed..169f6c43225 100644 --- a/app/views/projects/triggers/_form.html.haml +++ b/app/views/projects/triggers/_form.html.haml @@ -6,6 +6,27 @@ %label.label-light Token %p.form-control-static= @trigger.token .form-group - = f.label :key, "Description", class: "label-light" + = f.label :key, "Description (For extenral trigger and scheduled trigger)", class: "label-light" = f.text_field :description, class: "form-control", required: true, title: 'Trigger description is required.', placeholder: "Trigger description" + = f.fields_for :trigger_schedule do |schedule_fields| + .form-group + = schedule_fields.label :cron, "Cron (For scheduled trigger)", class: "label-light" + = schedule_fields.text_field :cron, class: "form-control", title: 'Trigger Schedule cron is required.', placeholder: "0 1 * * *" + .form-group + = schedule_fields.label :cron_timezone, "Cron timezone (For scheduled trigger)", class: "label-light" + = schedule_fields.text_field :cron_timezone, class: "form-control", title: 'Trigger Schedule cron_timezone is required.', placeholder: "UTC" + .form-group + = f.label :ref, "Ref (For scheduled trigger)", class: "label-light" + = f.text_field :ref, class: "form-control", title: 'Trigger Schedule Ref is required.', placeholder: "master" + - if action_name == 'edit' + = f.hidden_field :trigger_schedule_on, :value => @trigger.trigger_schedule.present? ? 1 : 0 + - else + .form-group + .checkbox + = f.label :trigger_schedule_on do + = f.check_box :trigger_schedule_on + %strong Register as scheduled trigger + .help-block + If checked, this trigger will be executed periodically according to `cron`, `cron_timezone` and `ref` + = link_to icon('question-circle'), help_page_path('user/project/pipelines/settings', anchor: 'visibility-of-pipelines') = f.submit btn_text, class: "btn btn-save" diff --git a/app/views/projects/triggers/_index.html.haml b/app/views/projects/triggers/_index.html.haml index cc74e50a5e3..84e945ee0df 100644 --- a/app/views/projects/triggers/_index.html.haml +++ b/app/views/projects/triggers/_index.html.haml @@ -22,6 +22,8 @@ %th %strong Last used %th + %strong Next run at + %th = render partial: 'projects/triggers/trigger', collection: @triggers, as: :trigger - else %p.settings-message.text-center.append-bottom-default diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 9b5f63ae81a..87d7c741bd6 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -29,6 +29,12 @@ - else Never + %td + - if trigger.trigger_schedule.present? + = trigger.trigger_schedule.real_next_run + - else + N/A (External trigger) + %td.text-right.trigger-actions - take_ownership_confirmation = "By taking ownership you will bind this trigger to your user account. With this the trigger will have access to all your projects as if it was you. Are you sure?" - revoke_trigger_confirmation = "By revoking a trigger you will break any processes making use of it. Are you sure?" From 76c0364ca52ce0b777f2ddb277fe666d95392256 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 7 Apr 2017 15:52:25 +0900 Subject: [PATCH 02/19] Use allow_destroy. Remove condtion from form.haml. --- app/controllers/projects/triggers_controller.rb | 10 ++++++---- app/models/ci/trigger.rb | 15 ++------------- app/models/ci/trigger_schedule.rb | 5 +++-- app/views/projects/triggers/_form.html.haml | 7 ++----- 4 files changed, 13 insertions(+), 24 deletions(-) diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index 5ceeeb1c454..2a3c563890b 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -16,6 +16,7 @@ class Projects::TriggersController < Projects::ApplicationController if @trigger.valid? flash[:notice] = 'Trigger was created successfully.' else + puts "@trigger.errors: #{@trigger.errors.inspect}" flash[:alert] = 'You could not create a new trigger.' end @@ -33,6 +34,7 @@ class Projects::TriggersController < Projects::ApplicationController end def edit + @trigger.build_trigger_schedule unless @trigger.trigger_schedule.present? end def update @@ -69,15 +71,15 @@ class Projects::TriggersController < Projects::ApplicationController def create_params params.require(:trigger).permit( - :description, :ref, :trigger_schedule_on, - trigger_schedule_attributes: [ :cron, :cron_timezone ] + :description, :ref, + trigger_schedule_attributes: [ :cron, :cron_timezone, :_destroy ] ) end def update_params params.require(:trigger).permit( - :description, :ref, :trigger_schedule_on, - trigger_schedule_attributes: [ :cron, :cron_timezone ] + :description, :ref, + trigger_schedule_attributes: [ :cron, :cron_timezone, :_destroy ] ) end end diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 225b6253616..f7ceba0d2a5 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -8,27 +8,16 @@ module Ci belongs_to :owner, class_name: "User" has_many :trigger_requests, dependent: :destroy - has_one :trigger_schedule, dependent: :destroy + has_one :trigger_schedule, dependent: :destroy, inverse_of: :trigger validates :token, presence: true, uniqueness: true before_validation :set_default_values - accepts_nested_attributes_for :trigger_schedule - - attr_accessor :trigger_schedule_on + accepts_nested_attributes_for :trigger_schedule, allow_destroy: true def set_default_values self.token = SecureRandom.hex(15) if self.token.blank? - - if trigger_schedule_on.present? - if trigger_schedule_on.to_i == 1 - self.trigger_schedule.project = project - self.trigger_schedule.trigger = self - else - self.trigger_schedule = nil - end - end end def last_trigger_request diff --git a/app/models/ci/trigger_schedule.rb b/app/models/ci/trigger_schedule.rb index d8a2770ea61..f7d602920ba 100644 --- a/app/models/ci/trigger_schedule.rb +++ b/app/models/ci/trigger_schedule.rb @@ -6,11 +6,11 @@ module Ci acts_as_paranoid belongs_to :project - belongs_to :trigger + belongs_to :trigger, inverse_of: :trigger_schedule delegate :ref, to: :trigger, allow_nil: true - validates :trigger, presence: { unless: :importing? } + validates_presence_of :trigger validates :cron, cron: true, presence: { unless: :importing? } validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? } validates :ref, presence: { unless: :importing? } @@ -19,6 +19,7 @@ module Ci def set_next_run_at self.next_run_at = Gitlab::Ci::CronParser.new(cron, cron_timezone).next_time_from(Time.now) + self.project = trigger.project end def schedule_next_run! diff --git a/app/views/projects/triggers/_form.html.haml b/app/views/projects/triggers/_form.html.haml index 169f6c43225..c56ee3a1130 100644 --- a/app/views/projects/triggers/_form.html.haml +++ b/app/views/projects/triggers/_form.html.haml @@ -18,13 +18,10 @@ .form-group = f.label :ref, "Ref (For scheduled trigger)", class: "label-light" = f.text_field :ref, class: "form-control", title: 'Trigger Schedule Ref is required.', placeholder: "master" - - if action_name == 'edit' - = f.hidden_field :trigger_schedule_on, :value => @trigger.trigger_schedule.present? ? 1 : 0 - - else .form-group .checkbox - = f.label :trigger_schedule_on do - = f.check_box :trigger_schedule_on + = schedule_fields.label :_destroy do + = schedule_fields.check_box :_destroy, { checked: (@trigger.trigger_schedule.id.present?) }, 0, 1 %strong Register as scheduled trigger .help-block If checked, this trigger will be executed periodically according to `cron`, `cron_timezone` and `ref` From ea8574fd3777ecf8eabf9483b14ee530337faf28 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 7 Apr 2017 18:29:17 +0900 Subject: [PATCH 03/19] Add specs in triggers_spec.rb (Halfway) --- spec/features/triggers_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index c1ae6db00c6..d317bb7bec7 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -125,6 +125,14 @@ feature 'Triggers', feature: true, js: true do describe 'show triggers workflow' do scenario 'contains trigger description placeholder' do expect(page.find('#trigger_description')['placeholder']).to eq 'Trigger description' + expect(page.find('#trigger_trigger_schedule_attributes_cron')['placeholder']).to eq '0 1 * * *' + expect(page.find('#trigger_trigger_schedule_attributes_cron_timezone')['placeholder']).to eq 'UTC' + expect(page.find('#trigger_ref')['placeholder']).to eq 'master' + end + + scenario 'show checkbox for registration of scheduled trigger and checked off defaultly' do + expect(page.find('#trigger_trigger_schedule_attributes__destroy')['type']).to eq 'checkbox' + expect(page.find('#trigger_trigger_schedule_attributes__destroy')['value']).to eq '0' end scenario 'show "legacy" badge for legacy trigger' do From 4131ed2bd85d3efdfe0e27d47ffc95567dae11af Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 7 Apr 2017 19:17:41 +0900 Subject: [PATCH 04/19] before_create :set_project. Now TriggerSchedule saves project from parent --- app/models/ci/trigger_schedule.rb | 6 +++++- spec/factories/ci/trigger_schedules.rb | 4 ---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/ci/trigger_schedule.rb b/app/models/ci/trigger_schedule.rb index f7d602920ba..bb58c510277 100644 --- a/app/models/ci/trigger_schedule.rb +++ b/app/models/ci/trigger_schedule.rb @@ -15,11 +15,15 @@ module Ci validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? } validates :ref, presence: { unless: :importing? } + before_create :set_project before_save :set_next_run_at + def set_project + self.project = trigger.project + end + def set_next_run_at self.next_run_at = Gitlab::Ci::CronParser.new(cron, cron_timezone).next_time_from(Time.now) - self.project = trigger.project end def schedule_next_run! diff --git a/spec/factories/ci/trigger_schedules.rb b/spec/factories/ci/trigger_schedules.rb index 315bce16995..f20799245e0 100644 --- a/spec/factories/ci/trigger_schedules.rb +++ b/spec/factories/ci/trigger_schedules.rb @@ -4,10 +4,6 @@ FactoryGirl.define do cron '0 1 * * *' cron_timezone Gitlab::Ci::CronParser::VALID_SYNTAX_SAMPLE_TIME_ZONE - after(:build) do |trigger_schedule, evaluator| - trigger_schedule.update!(project: trigger_schedule.trigger.project) - end - trait :nightly do cron '0 1 * * *' cron_timezone Gitlab::Ci::CronParser::VALID_SYNTAX_SAMPLE_TIME_ZONE From 2f5095c2546af30da152e85a499c98d608465988 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 7 Apr 2017 19:24:20 +0900 Subject: [PATCH 05/19] Add def trigger_schedule in Trigger. Use persisted? for checling existance --- app/controllers/projects/settings/ci_cd_controller.rb | 1 - app/controllers/projects/triggers_controller.rb | 1 - app/models/ci/trigger.rb | 4 ++++ app/views/projects/triggers/_trigger.html.haml | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 0ae4b4d3945..6f009d61950 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -27,7 +27,6 @@ module Projects def define_triggers_variables @triggers = @project.triggers @trigger = Ci::Trigger.new - @trigger.build_trigger_schedule end def define_badges_variables diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index 2a3c563890b..4f5d336ce40 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -34,7 +34,6 @@ class Projects::TriggersController < Projects::ApplicationController end def edit - @trigger.build_trigger_schedule unless @trigger.trigger_schedule.present? end def update diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index f7ceba0d2a5..3358f901542 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -39,5 +39,9 @@ module Ci def can_access_project? self.owner_id.blank? || Ability.allowed?(self.owner, :create_build, project) end + + def trigger_schedule + super || build_trigger_schedule + end end end diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 87d7c741bd6..9ab0cd3486c 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -30,7 +30,7 @@ Never %td - - if trigger.trigger_schedule.present? + - if trigger.trigger_schedule.persisted? = trigger.trigger_schedule.real_next_run - else N/A (External trigger) From fb8c49db8b16e45a33cff2839390bcbaab262075 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 7 Apr 2017 19:28:04 +0900 Subject: [PATCH 06/19] create_params and update_params into trigger_params --- app/controllers/projects/triggers_controller.rb | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index 4f5d336ce40..c1fc40f75d7 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -11,7 +11,7 @@ class Projects::TriggersController < Projects::ApplicationController end def create - @trigger = project.triggers.create(create_params.merge(owner: current_user)) + @trigger = project.triggers.create(trigger_params.merge(owner: current_user)) if @trigger.valid? flash[:notice] = 'Trigger was created successfully.' @@ -37,7 +37,7 @@ class Projects::TriggersController < Projects::ApplicationController end def update - if trigger.update(update_params) + if trigger.update(trigger_params) redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project), notice: 'Trigger was successfully updated.' else render action: "edit" @@ -68,17 +68,10 @@ class Projects::TriggersController < Projects::ApplicationController @trigger ||= project.triggers.find(params[:id]) || render_404 end - def create_params + def trigger_params params.require(:trigger).permit( :description, :ref, - trigger_schedule_attributes: [ :cron, :cron_timezone, :_destroy ] - ) - end - - def update_params - params.require(:trigger).permit( - :description, :ref, - trigger_schedule_attributes: [ :cron, :cron_timezone, :_destroy ] + trigger_schedule_attributes: [:cron, :cron_timezone, :_destroy] ) end end From 1ae1d85cdc835c8ed7907e83f3e371018c7c8fe6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 7 Apr 2017 20:01:14 +0900 Subject: [PATCH 07/19] N/A to None. Revert validates from validates_presence_of. --- app/models/ci/trigger_schedule.rb | 2 +- app/views/projects/triggers/_trigger.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/trigger_schedule.rb b/app/models/ci/trigger_schedule.rb index bb58c510277..72716eb416b 100644 --- a/app/models/ci/trigger_schedule.rb +++ b/app/models/ci/trigger_schedule.rb @@ -10,7 +10,7 @@ module Ci delegate :ref, to: :trigger, allow_nil: true - validates_presence_of :trigger + validates :trigger, presence: { unless: :importing? } validates :cron, cron: true, presence: { unless: :importing? } validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? } validates :ref, presence: { unless: :importing? } diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 9ab0cd3486c..3ed1fc9ac23 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -33,7 +33,7 @@ - if trigger.trigger_schedule.persisted? = trigger.trigger_schedule.real_next_run - else - N/A (External trigger) + None (External trigger) %td.text-right.trigger-actions - take_ownership_confirmation = "By taking ownership you will bind this trigger to your user account. With this the trigger will have access to all your projects as if it was you. Are you sure?" From 03088552549ed1631bb16c1bf3d0bef3613ec793 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 14:47:29 +0200 Subject: [PATCH 08/19] Fix ref reference --- .../projects/triggers_controller.rb | 5 +- app/models/ci/trigger.rb | 4 +- app/models/ci/trigger_schedule.rb | 17 +++--- app/views/projects/triggers/_form.html.haml | 53 ++++++++++++------- .../projects/triggers/_trigger.html.haml | 4 +- app/workers/trigger_schedule_worker.rb | 2 +- .../add-ui-for-trigger-schedule.yml | 4 ++ ...07114956_add_ref_to_ci_trigger_schedule.rb | 31 +++++++++++ ...22426_add_active_to_ci_trigger_schedule.rb | 31 +++++++++++ spec/factories/ci/trigger_schedules.rb | 4 ++ 10 files changed, 119 insertions(+), 36 deletions(-) create mode 100644 changelogs/unreleased/add-ui-for-trigger-schedule.yml create mode 100644 db/migrate/20170407114956_add_ref_to_ci_trigger_schedule.rb create mode 100644 db/migrate/20170407122426_add_active_to_ci_trigger_schedule.rb diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index c1fc40f75d7..afa56de920b 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -16,7 +16,6 @@ class Projects::TriggersController < Projects::ApplicationController if @trigger.valid? flash[:notice] = 'Trigger was created successfully.' else - puts "@trigger.errors: #{@trigger.errors.inspect}" flash[:alert] = 'You could not create a new trigger.' end @@ -70,8 +69,8 @@ class Projects::TriggersController < Projects::ApplicationController def trigger_params params.require(:trigger).permit( - :description, :ref, - trigger_schedule_attributes: [:cron, :cron_timezone, :_destroy] + :description, + trigger_schedule_attributes: [:id, :active, :cron, :cron_timezone, :ref] ) end end diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 3358f901542..d01fad44fda 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -8,7 +8,7 @@ module Ci belongs_to :owner, class_name: "User" has_many :trigger_requests, dependent: :destroy - has_one :trigger_schedule, dependent: :destroy, inverse_of: :trigger + has_one :trigger_schedule, dependent: :destroy validates :token, presence: true, uniqueness: true @@ -41,7 +41,7 @@ module Ci end def trigger_schedule - super || build_trigger_schedule + super || build_trigger_schedule(project: project) end end end diff --git a/app/models/ci/trigger_schedule.rb b/app/models/ci/trigger_schedule.rb index 72716eb416b..c9a16643f32 100644 --- a/app/models/ci/trigger_schedule.rb +++ b/app/models/ci/trigger_schedule.rb @@ -6,20 +6,19 @@ module Ci acts_as_paranoid belongs_to :project - belongs_to :trigger, inverse_of: :trigger_schedule - - delegate :ref, to: :trigger, allow_nil: true + belongs_to :trigger validates :trigger, presence: { unless: :importing? } - validates :cron, cron: true, presence: { unless: :importing? } - validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? } - validates :ref, presence: { unless: :importing? } + validates :cron, unless: :importing_or_inactive?, cron: true, presence: { unless: :importing_or_inactive? } + validates :cron_timezone, cron_timezone: true, presence: { unless: :importing_or_inactive? } + validates :ref, presence: { unless: :importing_or_inactive? } - before_create :set_project before_save :set_next_run_at - def set_project - self.project = trigger.project + scope :active, -> { where(active: true) } + + def importing_or_inactive? + importing? || !active? end def set_next_run_at diff --git a/app/views/projects/triggers/_form.html.haml b/app/views/projects/triggers/_form.html.haml index c56ee3a1130..410caeeecd4 100644 --- a/app/views/projects/triggers/_form.html.haml +++ b/app/views/projects/triggers/_form.html.haml @@ -6,24 +6,39 @@ %label.label-light Token %p.form-control-static= @trigger.token .form-group - = f.label :key, "Description (For extenral trigger and scheduled trigger)", class: "label-light" + = f.label :key, "Description", class: "label-light" = f.text_field :description, class: "form-control", required: true, title: 'Trigger description is required.', placeholder: "Trigger description" - = f.fields_for :trigger_schedule do |schedule_fields| - .form-group - = schedule_fields.label :cron, "Cron (For scheduled trigger)", class: "label-light" - = schedule_fields.text_field :cron, class: "form-control", title: 'Trigger Schedule cron is required.', placeholder: "0 1 * * *" - .form-group - = schedule_fields.label :cron_timezone, "Cron timezone (For scheduled trigger)", class: "label-light" - = schedule_fields.text_field :cron_timezone, class: "form-control", title: 'Trigger Schedule cron_timezone is required.', placeholder: "UTC" - .form-group - = f.label :ref, "Ref (For scheduled trigger)", class: "label-light" - = f.text_field :ref, class: "form-control", title: 'Trigger Schedule Ref is required.', placeholder: "master" - .form-group - .checkbox - = schedule_fields.label :_destroy do - = schedule_fields.check_box :_destroy, { checked: (@trigger.trigger_schedule.id.present?) }, 0, 1 - %strong Register as scheduled trigger - .help-block - If checked, this trigger will be executed periodically according to `cron`, `cron_timezone` and `ref` - = link_to icon('question-circle'), help_page_path('user/project/pipelines/settings', anchor: 'visibility-of-pipelines') + - if @trigger.persisted? + %hr + = f.fields_for :trigger_schedule do |schedule_fields| + = schedule_fields.hidden_field :id + .form-group + .checkbox + = schedule_fields.label :active do + = schedule_fields.check_box :active + %strong Schedule trigger + .help-block + If checked, this trigger will be executed periodically according to `cron`, `cron_timezone` and `ref` + = link_to icon('question-circle'), help_page_path('user/project/pipelines/settings', anchor: 'visibility-of-pipelines') + .form-group + = schedule_fields.label :cron, "Cron", class: "label-light" + = schedule_fields.text_field :cron, class: "form-control", title: 'Trigger Schedule cron is required.', placeholder: "0 1 * * *" + .form-group + = schedule_fields.label :cron, "Timezone", class: "label-light" + = schedule_fields.text_field :cron_timezone, class: "form-control", title: 'Trigger Schedule cron_timezone is required.', placeholder: "UTC" + .form-group + - schedule_ref = @trigger.trigger_schedule.ref || @project.default_branch + = schedule_fields.label :ref, "Branch or tag", class: "label-light" + = hidden_field_tag 'trigger[trigger_schedule_attributes][ref]', schedule_ref + = dropdown_tag(schedule_ref, + options: { toggle_class: 'js-branch-select wide', + filter: true, dropdown_class: "dropdown-menu-selectable", placeholder: "Search branches", + data: { selected: schedule_ref, field_name: 'trigger[trigger_schedule_attributes][ref]' } }) + .help-block Existing branch name, tag = f.submit btn_text, class: "btn btn-save" + + +:javascript + var availableRefs = #{@project.repository.ref_names.to_json}; + + new NewBranchForm($('.js-new-pipeline-form'), availableRefs) diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 3ed1fc9ac23..71fb37928fe 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -30,10 +30,10 @@ Never %td - - if trigger.trigger_schedule.persisted? + - if trigger.trigger_schedule.present? && trigger.trigger_schedule.active? = trigger.trigger_schedule.real_next_run - else - None (External trigger) + None %td.text-right.trigger-actions - take_ownership_confirmation = "By taking ownership you will bind this trigger to your user account. With this the trigger will have access to all your projects as if it was you. Are you sure?" diff --git a/app/workers/trigger_schedule_worker.rb b/app/workers/trigger_schedule_worker.rb index 440c579b99d..9c1baf7e6c5 100644 --- a/app/workers/trigger_schedule_worker.rb +++ b/app/workers/trigger_schedule_worker.rb @@ -3,7 +3,7 @@ class TriggerScheduleWorker include CronjobQueue def perform - Ci::TriggerSchedule.where("next_run_at < ?", Time.now).find_each do |trigger_schedule| + Ci::TriggerSchedule.active.where("next_run_at < ?", Time.now).find_each do |trigger_schedule| begin Ci::CreateTriggerRequestService.new.execute(trigger_schedule.project, trigger_schedule.trigger, diff --git a/changelogs/unreleased/add-ui-for-trigger-schedule.yml b/changelogs/unreleased/add-ui-for-trigger-schedule.yml new file mode 100644 index 00000000000..e60a0975744 --- /dev/null +++ b/changelogs/unreleased/add-ui-for-trigger-schedule.yml @@ -0,0 +1,4 @@ +--- +title: Add UI for Trigger Schedule +merge_request: +author: diff --git a/db/migrate/20170407114956_add_ref_to_ci_trigger_schedule.rb b/db/migrate/20170407114956_add_ref_to_ci_trigger_schedule.rb new file mode 100644 index 00000000000..9bf82e49aec --- /dev/null +++ b/db/migrate/20170407114956_add_ref_to_ci_trigger_schedule.rb @@ -0,0 +1,31 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddRefToCiTriggerSchedule < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + # DOWNTIME_REASON = '' + + # When using the methods "add_concurrent_index", "remove_concurrent_index" or + # "add_column_with_default" you must disable the use of transactions + # as these methods can not run in an existing transaction. + # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure + # that either of them is the _only_ method called in the migration, + # any other changes should go in a separate migration. + # This ensures that upon failure _only_ the index creation or removing fails + # and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + + def change + add_column :ci_trigger_schedules, :ref, :string + end +end diff --git a/db/migrate/20170407122426_add_active_to_ci_trigger_schedule.rb b/db/migrate/20170407122426_add_active_to_ci_trigger_schedule.rb new file mode 100644 index 00000000000..1da31c66fa6 --- /dev/null +++ b/db/migrate/20170407122426_add_active_to_ci_trigger_schedule.rb @@ -0,0 +1,31 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddActiveToCiTriggerSchedule < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + # DOWNTIME_REASON = '' + + # When using the methods "add_concurrent_index", "remove_concurrent_index" or + # "add_column_with_default" you must disable the use of transactions + # as these methods can not run in an existing transaction. + # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure + # that either of them is the _only_ method called in the migration, + # any other changes should go in a separate migration. + # This ensures that upon failure _only_ the index creation or removing fails + # and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + + def change + add_column :ci_trigger_schedules, :active, :boolean + end +end diff --git a/spec/factories/ci/trigger_schedules.rb b/spec/factories/ci/trigger_schedules.rb index f20799245e0..315bce16995 100644 --- a/spec/factories/ci/trigger_schedules.rb +++ b/spec/factories/ci/trigger_schedules.rb @@ -4,6 +4,10 @@ FactoryGirl.define do cron '0 1 * * *' cron_timezone Gitlab::Ci::CronParser::VALID_SYNTAX_SAMPLE_TIME_ZONE + after(:build) do |trigger_schedule, evaluator| + trigger_schedule.update!(project: trigger_schedule.trigger.project) + end + trait :nightly do cron '0 1 * * *' cron_timezone Gitlab::Ci::CronParser::VALID_SYNTAX_SAMPLE_TIME_ZONE From 21a7aed9a7330382300bc93b8927609e65ee6390 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 16:02:13 +0200 Subject: [PATCH 09/19] Update tests to cover trigger schedule --- app/views/projects/triggers/_form.html.haml | 17 +++--- db/schema.rb | 4 +- spec/factories/ci/trigger_schedules.rb | 4 +- spec/features/triggers_spec.rb | 55 +++++++++++++++++--- spec/workers/trigger_schedule_worker_spec.rb | 34 ++++++++---- 5 files changed, 82 insertions(+), 32 deletions(-) diff --git a/app/views/projects/triggers/_form.html.haml b/app/views/projects/triggers/_form.html.haml index 410caeeecd4..41e73f1e822 100644 --- a/app/views/projects/triggers/_form.html.haml +++ b/app/views/projects/triggers/_form.html.haml @@ -16,24 +16,19 @@ .checkbox = schedule_fields.label :active do = schedule_fields.check_box :active - %strong Schedule trigger + %strong Schedule trigger (experimental) .help-block - If checked, this trigger will be executed periodically according to `cron`, `cron_timezone` and `ref` - = link_to icon('question-circle'), help_page_path('user/project/pipelines/settings', anchor: 'visibility-of-pipelines') + If checked, this trigger will be executed periodically according to `Cron` and `Timezone`. + = link_to icon('question-circle'), help_page_path('ci/triggers', anchor: 'schedule') .form-group = schedule_fields.label :cron, "Cron", class: "label-light" - = schedule_fields.text_field :cron, class: "form-control", title: 'Trigger Schedule cron is required.', placeholder: "0 1 * * *" + = schedule_fields.text_field :cron, class: "form-control", title: 'Cron specification is required.', placeholder: "0 1 * * *" .form-group = schedule_fields.label :cron, "Timezone", class: "label-light" - = schedule_fields.text_field :cron_timezone, class: "form-control", title: 'Trigger Schedule cron_timezone is required.', placeholder: "UTC" + = schedule_fields.text_field :cron_timezone, class: "form-control", title: 'Timezone is required.', placeholder: "UTC" .form-group - - schedule_ref = @trigger.trigger_schedule.ref || @project.default_branch = schedule_fields.label :ref, "Branch or tag", class: "label-light" - = hidden_field_tag 'trigger[trigger_schedule_attributes][ref]', schedule_ref - = dropdown_tag(schedule_ref, - options: { toggle_class: 'js-branch-select wide', - filter: true, dropdown_class: "dropdown-menu-selectable", placeholder: "Search branches", - data: { selected: schedule_ref, field_name: 'trigger[trigger_schedule_attributes][ref]' } }) + = schedule_fields.text_field :ref, class: "form-control", title: 'Branch or tag is required.', placeholder: "master" .help-block Existing branch name, tag = f.submit btn_text, class: "btn btn-save" diff --git a/db/schema.rb b/db/schema.rb index 16f3f293079..b74aebc8305 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: 20170406115029) do +ActiveRecord::Schema.define(version: 20170407122426) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -311,6 +311,8 @@ ActiveRecord::Schema.define(version: 20170406115029) do t.string "cron" t.string "cron_timezone" t.datetime "next_run_at" + t.string "ref" + t.boolean "active" end add_index "ci_trigger_schedules", ["next_run_at"], name: "index_ci_trigger_schedules_on_next_run_at", using: :btree diff --git a/spec/factories/ci/trigger_schedules.rb b/spec/factories/ci/trigger_schedules.rb index 315bce16995..2390706fa41 100644 --- a/spec/factories/ci/trigger_schedules.rb +++ b/spec/factories/ci/trigger_schedules.rb @@ -3,9 +3,11 @@ FactoryGirl.define do trigger factory: :ci_trigger_for_trigger_schedule cron '0 1 * * *' cron_timezone Gitlab::Ci::CronParser::VALID_SYNTAX_SAMPLE_TIME_ZONE + ref 'master' + active true after(:build) do |trigger_schedule, evaluator| - trigger_schedule.update!(project: trigger_schedule.trigger.project) + trigger_schedule.project ||= trigger_schedule.trigger.project end trait :nightly do diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index d317bb7bec7..10bfb9098c7 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -77,6 +77,53 @@ feature 'Triggers', feature: true, js: true do expect(page.find('.flash-notice')).to have_content 'Trigger was successfully updated.' expect(page.find('.triggers-list')).to have_content new_trigger_title end + + context 'scheduled triggers' do + let!(:trigger) do + create(:ci_trigger, owner: user, project: @project, description: trigger_title) + end + + context 'enabling schedule' do + before do + visit edit_namespace_project_trigger_path(@project.namespace, @project, trigger) + end + + scenario 'do fill form with valid data and save' do + find('#trigger_trigger_schedule_attributes_active').click + fill_in 'trigger_trigger_schedule_attributes_cron', with: '1 * * * *' + fill_in 'trigger_trigger_schedule_attributes_cron_timezone', with: 'UTC' + fill_in 'trigger_trigger_schedule_attributes_ref', with: 'master' + click_button 'Save trigger' + + expect(page.find('.flash-notice')).to have_content 'Trigger was successfully updated.' + end + + scenario 'do not fill form with valid data and save' do + find('#trigger_trigger_schedule_attributes_active').click + click_button 'Save trigger' + + expect(page).to have_content 'The form contains the following errors' + end + end + + context 'enabling schedule' do + before do + trigger.create_trigger_schedule(project: trigger.project, active: true) + + visit edit_namespace_project_trigger_path(@project.namespace, @project, trigger) + end + + scenario 'disable and save form' do + find('#trigger_trigger_schedule_attributes_active').click + click_button 'Save trigger' + expect(page.find('.flash-notice')).to have_content 'Trigger was successfully updated.' + + visit edit_namespace_project_trigger_path(@project.namespace, @project, trigger) + checkbox = find_field('trigger_trigger_schedule_attributes_active') + expect(checkbox).not_to be_checked + end + end + end end describe 'trigger "Take ownership" workflow' do @@ -125,14 +172,6 @@ feature 'Triggers', feature: true, js: true do describe 'show triggers workflow' do scenario 'contains trigger description placeholder' do expect(page.find('#trigger_description')['placeholder']).to eq 'Trigger description' - expect(page.find('#trigger_trigger_schedule_attributes_cron')['placeholder']).to eq '0 1 * * *' - expect(page.find('#trigger_trigger_schedule_attributes_cron_timezone')['placeholder']).to eq 'UTC' - expect(page.find('#trigger_ref')['placeholder']).to eq 'master' - end - - scenario 'show checkbox for registration of scheduled trigger and checked off defaultly' do - expect(page.find('#trigger_trigger_schedule_attributes__destroy')['type']).to eq 'checkbox' - expect(page.find('#trigger_trigger_schedule_attributes__destroy')['value']).to eq '0' end scenario 'show "legacy" badge for legacy trigger' do diff --git a/spec/workers/trigger_schedule_worker_spec.rb b/spec/workers/trigger_schedule_worker_spec.rb index 151e1c2f7b9..fdc638d9070 100644 --- a/spec/workers/trigger_schedule_worker_spec.rb +++ b/spec/workers/trigger_schedule_worker_spec.rb @@ -8,24 +8,39 @@ describe TriggerScheduleWorker do end context 'when there is a scheduled trigger within next_run_at' do + let(:next_run_at) { 2.days.ago } + + let!(:trigger_schedule) do + create(:ci_trigger_schedule, :nightly) + end + before do - trigger_schedule = create(:ci_trigger_schedule, :nightly) - time_future = Time.now + 10.days - allow(Time).to receive(:now).and_return(time_future) - @next_time = Gitlab::Ci::CronParser.new(trigger_schedule.cron, trigger_schedule.cron_timezone).next_time_from(time_future) + trigger_schedule.update_column(:next_run_at, next_run_at) end it 'creates a new trigger request' do - expect { worker.perform }.to change { Ci::TriggerRequest.count }.by(1) + expect { worker.perform }.to change { Ci::TriggerRequest.count } end it 'creates a new pipeline' do - expect { worker.perform }.to change { Ci::Pipeline.count }.by(1) + expect { worker.perform }.to change { Ci::Pipeline.count } expect(Ci::Pipeline.last).to be_pending end it 'updates next_run_at' do - expect { worker.perform }.to change { Ci::TriggerSchedule.last.next_run_at }.to(@next_time) + worker.perform + + expect(trigger_schedule.reload.next_run_at).not_to eq(next_run_at) + end + + context 'inactive schedule' do + before do + trigger_schedule.update(active: false) + end + + it 'does not create a new trigger' do + expect { worker.perform }.not_to change { Ci::TriggerRequest.count } + end end end @@ -42,10 +57,7 @@ describe TriggerScheduleWorker do end context 'when next_run_at is nil' do - before do - trigger_schedule = create(:ci_trigger_schedule, :nightly) - trigger_schedule.update_attribute(:next_run_at, nil) - end + let!(:trigger_schedule) { create(:ci_trigger_schedule, :nightly, next_run_at: nil) } it 'does not create a new pipeline' do expect { worker.perform }.not_to change { Ci::Pipeline.count } From 7cee650d0c75c0b9c229965711437779b847130f Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 16:06:51 +0200 Subject: [PATCH 10/19] Update migrations --- ...07114956_add_ref_to_ci_trigger_schedule.rb | 22 ------------------- ...22426_add_active_to_ci_trigger_schedule.rb | 22 ------------------- ...450_add_index_to_next_run_at_and_active.rb | 12 ++++++++++ db/schema.rb | 3 ++- 4 files changed, 14 insertions(+), 45 deletions(-) create mode 100644 db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb diff --git a/db/migrate/20170407114956_add_ref_to_ci_trigger_schedule.rb b/db/migrate/20170407114956_add_ref_to_ci_trigger_schedule.rb index 9bf82e49aec..523a306f127 100644 --- a/db/migrate/20170407114956_add_ref_to_ci_trigger_schedule.rb +++ b/db/migrate/20170407114956_add_ref_to_ci_trigger_schedule.rb @@ -1,30 +1,8 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - class AddRefToCiTriggerSchedule < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers - # Set this constant to true if this migration requires downtime. DOWNTIME = false - # When a migration requires downtime you **must** uncomment the following - # constant and define a short and easy to understand explanation as to why the - # migration requires downtime. - # DOWNTIME_REASON = '' - - # When using the methods "add_concurrent_index", "remove_concurrent_index" or - # "add_column_with_default" you must disable the use of transactions - # as these methods can not run in an existing transaction. - # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure - # that either of them is the _only_ method called in the migration, - # any other changes should go in a separate migration. - # This ensures that upon failure _only_ the index creation or removing fails - # and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - def change add_column :ci_trigger_schedules, :ref, :string end diff --git a/db/migrate/20170407122426_add_active_to_ci_trigger_schedule.rb b/db/migrate/20170407122426_add_active_to_ci_trigger_schedule.rb index 1da31c66fa6..36892118ac0 100644 --- a/db/migrate/20170407122426_add_active_to_ci_trigger_schedule.rb +++ b/db/migrate/20170407122426_add_active_to_ci_trigger_schedule.rb @@ -1,30 +1,8 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - class AddActiveToCiTriggerSchedule < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers - # Set this constant to true if this migration requires downtime. DOWNTIME = false - # When a migration requires downtime you **must** uncomment the following - # constant and define a short and easy to understand explanation as to why the - # migration requires downtime. - # DOWNTIME_REASON = '' - - # When using the methods "add_concurrent_index", "remove_concurrent_index" or - # "add_column_with_default" you must disable the use of transactions - # as these methods can not run in an existing transaction. - # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure - # that either of them is the _only_ method called in the migration, - # any other changes should go in a separate migration. - # This ensures that upon failure _only_ the index creation or removing fails - # and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - def change add_column :ci_trigger_schedules, :active, :boolean end diff --git a/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb b/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb new file mode 100644 index 00000000000..887632da20e --- /dev/null +++ b/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb @@ -0,0 +1,12 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndexToNextRunAtAndActive < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_index :ci_trigger_schedules, [:active, :next_run_at] + end +end diff --git a/db/schema.rb b/db/schema.rb index b74aebc8305..2854416a8ef 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: 20170407122426) do +ActiveRecord::Schema.define(version: 20170407140450) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -315,6 +315,7 @@ ActiveRecord::Schema.define(version: 20170407122426) do t.boolean "active" end + add_index "ci_trigger_schedules", ["active", "next_run_at"], name: "index_ci_trigger_schedules_on_active_and_next_run_at", using: :btree add_index "ci_trigger_schedules", ["next_run_at"], name: "index_ci_trigger_schedules_on_next_run_at", using: :btree add_index "ci_trigger_schedules", ["project_id"], name: "index_ci_trigger_schedules_on_project_id", using: :btree From 7461072f36d88254b2bc5d696061c7c0f5b8fe86 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 16:13:21 +0200 Subject: [PATCH 11/19] Update code to remove no longer needed changes --- app/models/ci/trigger.rb | 2 +- app/views/projects/triggers/_form.html.haml | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index d01fad44fda..b59e235c425 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -14,7 +14,7 @@ module Ci before_validation :set_default_values - accepts_nested_attributes_for :trigger_schedule, allow_destroy: true + accepts_nested_attributes_for :trigger_schedule def set_default_values self.token = SecureRandom.hex(15) if self.token.blank? diff --git a/app/views/projects/triggers/_form.html.haml b/app/views/projects/triggers/_form.html.haml index 41e73f1e822..8ec2f05e3d6 100644 --- a/app/views/projects/triggers/_form.html.haml +++ b/app/views/projects/triggers/_form.html.haml @@ -31,9 +31,3 @@ = schedule_fields.text_field :ref, class: "form-control", title: 'Branch or tag is required.', placeholder: "master" .help-block Existing branch name, tag = f.submit btn_text, class: "btn btn-save" - - -:javascript - var availableRefs = #{@project.repository.ref_names.to_json}; - - new NewBranchForm($('.js-new-pipeline-form'), availableRefs) From 344001d7384ec227fdf18cc87b62981f18c7c580 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 16:20:00 +0200 Subject: [PATCH 12/19] Simplify if --- app/views/projects/triggers/_trigger.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 71fb37928fe..ca4549fc557 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -30,7 +30,7 @@ Never %td - - if trigger.trigger_schedule.present? && trigger.trigger_schedule.active? + - if trigger.trigger_schedule&.active? = trigger.trigger_schedule.real_next_run - else None From d5201119ee2a139c18b51babd80f6c6490f90b6e Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 17:15:28 +0200 Subject: [PATCH 13/19] Fix test failures --- app/models/ci/trigger_schedule.rb | 5 +++-- app/views/projects/triggers/_form.html.haml | 2 +- app/views/projects/triggers/_trigger.html.haml | 2 +- spec/features/triggers_spec.rb | 1 + 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/ci/trigger_schedule.rb b/app/models/ci/trigger_schedule.rb index c9a16643f32..012a18eb439 100644 --- a/app/models/ci/trigger_schedule.rb +++ b/app/models/ci/trigger_schedule.rb @@ -31,8 +31,9 @@ module Ci update_attribute(:next_run_at, nil) # update without validation end - def real_next_run(worker_cron: Settings.cron_jobs['trigger_schedule_worker']['cron'], - worker_time_zone: Time.zone.name) + def real_next_run( + worker_cron: Settings.cron_jobs['trigger_schedule_worker']['cron'], + worker_time_zone: Time.zone.name) Gitlab::Ci::CronParser.new(worker_cron, worker_time_zone) .next_time_from(next_run_at) end diff --git a/app/views/projects/triggers/_form.html.haml b/app/views/projects/triggers/_form.html.haml index 8ec2f05e3d6..8582bcbb8cc 100644 --- a/app/views/projects/triggers/_form.html.haml +++ b/app/views/projects/triggers/_form.html.haml @@ -18,7 +18,7 @@ = schedule_fields.check_box :active %strong Schedule trigger (experimental) .help-block - If checked, this trigger will be executed periodically according to `Cron` and `Timezone`. + If checked, this trigger will be executed periodically according to cron and timezone. = link_to icon('question-circle'), help_page_path('ci/triggers', anchor: 'schedule') .form-group = schedule_fields.label :cron, "Cron", class: "label-light" diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index ca4549fc557..ebd91a8e2af 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -33,7 +33,7 @@ - if trigger.trigger_schedule&.active? = trigger.trigger_schedule.real_next_run - else - None + Never %td.text-right.trigger-actions - take_ownership_confirmation = "By taking ownership you will bind this trigger to your user account. With this the trigger will have access to all your projects as if it was you. Are you sure?" diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index 10bfb9098c7..906842583e4 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -120,6 +120,7 @@ feature 'Triggers', feature: true, js: true do visit edit_namespace_project_trigger_path(@project.namespace, @project, trigger) checkbox = find_field('trigger_trigger_schedule_attributes_active') + expect(checkbox).not_to be_checked end end From 8d3f7979cc5419cc8acca9478baa96c5bfeb4840 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 17:17:36 +0200 Subject: [PATCH 14/19] Fix changelog --- changelogs/unreleased/add-ui-for-trigger-schedule.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelogs/unreleased/add-ui-for-trigger-schedule.yml b/changelogs/unreleased/add-ui-for-trigger-schedule.yml index e60a0975744..9ca78983605 100644 --- a/changelogs/unreleased/add-ui-for-trigger-schedule.yml +++ b/changelogs/unreleased/add-ui-for-trigger-schedule.yml @@ -1,4 +1,4 @@ --- title: Add UI for Trigger Schedule -merge_request: -author: +merge_request: 10533 +author: dosuken123 From 0872b854cb4861351893ad78c4d4a86c61a2eccf Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 17:34:41 +0200 Subject: [PATCH 15/19] Another round of code review --- ...170407140450_add_index_to_next_run_at_and_active.rb | 10 ++++++++-- spec/features/triggers_spec.rb | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb b/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb index 887632da20e..e1382318d3c 100644 --- a/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb +++ b/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb @@ -6,7 +6,13 @@ class AddIndexToNextRunAtAndActive < ActiveRecord::Migration DOWNTIME = false - def change - add_index :ci_trigger_schedules, [:active, :next_run_at] + disable_ddl_transaction! + + def up + add_concurrent_index :ci_trigger_schedules, [:active, :next_run_at] + end + + def down + remove_index :ci_trigger_schedules, [:active, :next_run_at] end end diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index 906842583e4..da7535979a2 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -106,7 +106,7 @@ feature 'Triggers', feature: true, js: true do end end - context 'enabling schedule' do + context 'disabling schedule' do before do trigger.create_trigger_schedule(project: trigger.project, active: true) @@ -120,7 +120,7 @@ feature 'Triggers', feature: true, js: true do visit edit_namespace_project_trigger_path(@project.namespace, @project, trigger) checkbox = find_field('trigger_trigger_schedule_attributes_active') - + expect(checkbox).not_to be_checked end end From 3ea4a13925d7989288101ba40b25b02fa49ae23c Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 18:05:39 +0200 Subject: [PATCH 16/19] Yet, another errors --- .../20170407140450_add_index_to_next_run_at_and_active.rb | 2 +- spec/workers/trigger_schedule_worker_spec.rb | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb b/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb index e1382318d3c..626c2a67fdc 100644 --- a/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb +++ b/db/migrate/20170407140450_add_index_to_next_run_at_and_active.rb @@ -13,6 +13,6 @@ class AddIndexToNextRunAtAndActive < ActiveRecord::Migration end def down - remove_index :ci_trigger_schedules, [:active, :next_run_at] + remove_concurrent_index :ci_trigger_schedules, [:active, :next_run_at] end end diff --git a/spec/workers/trigger_schedule_worker_spec.rb b/spec/workers/trigger_schedule_worker_spec.rb index fdc638d9070..b34ef3175bf 100644 --- a/spec/workers/trigger_schedule_worker_spec.rb +++ b/spec/workers/trigger_schedule_worker_spec.rb @@ -57,7 +57,10 @@ describe TriggerScheduleWorker do end context 'when next_run_at is nil' do - let!(:trigger_schedule) { create(:ci_trigger_schedule, :nightly, next_run_at: nil) } + before + schedule = create(:ci_trigger_schedule, :nightly) + schedule.update_column(:next_run_at, nil) + end it 'does not create a new pipeline' do expect { worker.perform }.not_to change { Ci::Pipeline.count } From ed368f8c5ee54903f0148ea15e1e231f2683f1a8 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 18:52:24 +0200 Subject: [PATCH 17/19] Fix spec failure --- spec/workers/trigger_schedule_worker_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/workers/trigger_schedule_worker_spec.rb b/spec/workers/trigger_schedule_worker_spec.rb index b34ef3175bf..861bed4442e 100644 --- a/spec/workers/trigger_schedule_worker_spec.rb +++ b/spec/workers/trigger_schedule_worker_spec.rb @@ -57,7 +57,7 @@ describe TriggerScheduleWorker do end context 'when next_run_at is nil' do - before + before do schedule = create(:ci_trigger_schedule, :nightly) schedule.update_column(:next_run_at, nil) end From 4465411bdf7afd5f69b0f68110609de28ed1cca2 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 7 Apr 2017 19:18:29 +0200 Subject: [PATCH 18/19] Fix last spec failure --- spec/features/triggers_spec.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index da7535979a2..81fa2de1cc3 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -108,7 +108,12 @@ feature 'Triggers', feature: true, js: true do context 'disabling schedule' do before do - trigger.create_trigger_schedule(project: trigger.project, active: true) + trigger.create_trigger_schedule( + project: trigger.project, + active: true, + ref: 'master', + cron: '1 * * * *', + cron_timezone: 'UTC') visit edit_namespace_project_trigger_path(@project.namespace, @project, trigger) end From 682748e8abbc05d3059c2f945cda9d18081947f5 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 7 Apr 2017 19:29:24 +0100 Subject: [PATCH 19/19] Add ref and active to import/export config --- spec/lib/gitlab/import_export/safe_model_attributes.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 04b5c80f22d..14933dac957 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -253,6 +253,8 @@ Ci::TriggerSchedule: - cron - cron_timezone - next_run_at +- ref +- active DeployKey: - id - user_id