From f289decf9abfa1ff372fc12a58bf39cd3207804d Mon Sep 17 00:00:00 2001 From: Manon Deloupy Date: Tue, 11 Jun 2019 12:15:22 -0400 Subject: [PATCH 1/4] Add lock_level property to stacks This allows lock messages to be displayed as warnings in the banner without actually locking deploys. --- .../shipit/api/locks_controller.rb | 6 ++++-- app/models/shipit/stack.rb | 14 ++++++++++--- app/serializers/shipit/stack_serializer.rb | 8 +++---- app/views/shipit/stacks/_banners.html.erb | 16 ++++++++++++++ ...20190611152822_add_lock_level_to_stacks.rb | 5 +++++ test/controllers/api/locks_controller_test.rb | 21 +++++++++++++++++++ test/dummy/db/schema.rb | 11 +++++----- 7 files changed, 67 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20190611152822_add_lock_level_to_stacks.rb diff --git a/app/controllers/shipit/api/locks_controller.rb b/app/controllers/shipit/api/locks_controller.rb index e8fa95d8e..472ee4d2f 100644 --- a/app/controllers/shipit/api/locks_controller.rb +++ b/app/controllers/shipit/api/locks_controller.rb @@ -5,21 +5,23 @@ class LocksController < BaseController params do requires :reason, String, presence: true + accepts :lock_level, String end def create if stack.locked? render json: {message: 'Already locked'}, status: :conflict else - stack.lock(params.reason, current_user) + stack.lock(params.reason, current_user, lock_level: params.lock_level) render_resource stack end end params do requires :reason, String, presence: true + accepts :lock_level, String end def update - stack.lock(params.reason, current_user) + stack.lock(params.reason, current_user, lock_level: params.lock_level) render_resource stack end diff --git a/app/models/shipit/stack.rb b/app/models/shipit/stack.rb index 778cba46a..1629ee4ee 100644 --- a/app/models/shipit/stack.rb +++ b/app/models/shipit/stack.rb @@ -23,6 +23,9 @@ def blank? ENVIRONMENT_MAX_SIZE = 50 REQUIRED_HOOKS = %i(push status).freeze + LOCK_LEVELS = %w(enforced advisory).freeze + enum lock_level: LOCK_LEVELS.zip(LOCK_LEVELS).to_h + has_many :commits, dependent: :destroy has_many :pull_requests, dependent: :destroy has_many :tasks, dependent: :destroy @@ -386,16 +389,21 @@ def active_task end def locked? + lock_reason.present? && lock_level != 'advisory' + end + + def soft_locked? lock_reason.present? end - def lock(reason, user) - params = {lock_reason: reason, lock_author: user} + def lock(reason, user, lock_level: 'enforced') + lock_level ||= 'enforced' + params = { lock_reason: reason, lock_author: user, lock_level: lock_level } update!(params) end def unlock - update!(lock_reason: nil, lock_author: nil, locked_since: nil) + update!(lock_reason: nil, lock_author: nil, locked_since: nil, lock_level: nil) end def to_param diff --git a/app/serializers/shipit/stack_serializer.rb b/app/serializers/shipit/stack_serializer.rb index f695ed2f6..d9780e4ea 100644 --- a/app/serializers/shipit/stack_serializer.rb +++ b/app/serializers/shipit/stack_serializer.rb @@ -5,7 +5,7 @@ class StackSerializer < ActiveModel::Serializer has_one :lock_author attributes :id, :repo_owner, :repo_name, :environment, :html_url, :url, :tasks_url, :deploy_url, :pull_requests_url, :deploy_spec, :undeployed_commits_count, :is_locked, :lock_reason, :continuous_deployment, :created_at, - :updated_at, :locked_since, :last_deployed_at, :branch, :merge_queue_enabled + :updated_at, :locked_since, :last_deployed_at, :branch, :merge_queue_enabled, :lock_level def url api_stack_url(object) @@ -28,15 +28,15 @@ def is_locked end def include_lock_reason? - object.locked? + object.soft_locked? end def include_lock_author? - object.locked? + object.soft_locked? end def include_locked_since? - object.locked? + object.soft_locked? end def deploy_spec diff --git a/app/views/shipit/stacks/_banners.html.erb b/app/views/shipit/stacks/_banners.html.erb index 0e17fd2b5..8a2466ee5 100644 --- a/app/views/shipit/stacks/_banners.html.erb +++ b/app/views/shipit/stacks/_banners.html.erb @@ -47,6 +47,22 @@ +<% elsif stack.soft_locked? %> + <% end %> <% if stack.continuous_delivery_delayed? %> diff --git a/db/migrate/20190611152822_add_lock_level_to_stacks.rb b/db/migrate/20190611152822_add_lock_level_to_stacks.rb new file mode 100644 index 000000000..e7b8c7768 --- /dev/null +++ b/db/migrate/20190611152822_add_lock_level_to_stacks.rb @@ -0,0 +1,5 @@ +class AddLockLevelToStacks < ActiveRecord::Migration[5.2] + def change + add_column :stacks, :lock_level, :string + end +end diff --git a/test/controllers/api/locks_controller_test.rb b/test/controllers/api/locks_controller_test.rb index ec7037e18..804e53a5c 100644 --- a/test/controllers/api/locks_controller_test.rb +++ b/test/controllers/api/locks_controller_test.rb @@ -13,6 +13,16 @@ class LocksControllerTest < ActionController::TestCase assert_response :ok assert_json 'is_locked', true assert_json 'lock_reason', 'Just for fun!' + assert_json 'lock_level', 'enforced' + assert_json { |json| assert_not_nil json['locked_since'] } + end + + test "#create sets a lock message if lock_level is advisory" do + post :create, params: { stack_id: @stack.to_param, reason: 'Just for fun!', lock_level: 'advisory' } + assert_response :ok + assert_json 'is_locked', false + assert_json 'lock_reason', 'Just for fun!' + assert_json 'lock_level', 'advisory' assert_json { |json| assert_not_nil json['locked_since'] } end @@ -27,6 +37,7 @@ class LocksControllerTest < ActionController::TestCase assert_response :ok assert_json 'is_locked', true assert_json 'lock_reason', 'Just for fun!' + assert_json 'lock_level', 'enforced' end test "#update can override a previous lock" do @@ -35,6 +46,15 @@ class LocksControllerTest < ActionController::TestCase assert_response :ok assert_json 'is_locked', true assert_json 'lock_reason', 'Just for fun!' + assert_json 'lock_level', 'enforced' + end + + test "#update sets a lock with a lock_level if passed as a param" do + put :update, params: { stack_id: @stack.to_param, reason: 'Just for fun!', lock_level: 'advisory' } + assert_response :ok + assert_json 'is_locked', false + assert_json 'lock_reason', 'Just for fun!' + assert_json 'lock_level', 'advisory' end test "#update does not override previous locked_since" do @@ -50,6 +70,7 @@ class LocksControllerTest < ActionController::TestCase delete :destroy, params: {stack_id: @stack.to_param} assert_response :ok assert_json 'is_locked', false + assert_json 'lock_level', nil assert_json { |json| assert_nil json['locked_since'] } end end diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb index 5e22302b2..a72de081d 100644 --- a/test/dummy/db/schema.rb +++ b/test/dummy/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_05_02_020249) do +ActiveRecord::Schema.define(version: 2019_06_11_152822) do create_table "api_clients", force: :cascade do |t| t.text "permissions", limit: 65535 @@ -185,9 +185,9 @@ t.bigint "github_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["commit_id", "github_id"], name: "index_deploy_statuses_on_commit_id_and_github_id" - t.index ["stack_id", "commit_id"], name: "index_deploy_statuses_on_stack_id_and_commit_id" - t.index ["user_id"], name: "index_deploy_statuses_on_user_id" + t.index ["commit_id", "github_id"], name: "index_release_statuses_on_commit_id_and_github_id" + t.index ["stack_id", "commit_id"], name: "index_release_statuses_on_stack_id_and_commit_id" + t.index ["user_id"], name: "index_release_statuses_on_user_id" end create_table "stacks", force: :cascade do |t| @@ -211,6 +211,7 @@ t.datetime "locked_since" t.boolean "merge_queue_enabled", default: false, null: false t.datetime "last_deployed_at" + t.string "lock_level" t.index ["repo_owner", "repo_name", "environment"], name: "stack_unicity", unique: true end @@ -240,7 +241,7 @@ t.integer "additions", limit: 4, default: 0 t.integer "deletions", limit: 4, default: 0 t.text "definition", limit: 65535 - t.binary "gzip_output" + t.binary "gzip_output", limit: 16777215 t.boolean "rollback_once_aborted", default: false, null: false t.text "env" t.integer "confirmations", default: 0, null: false From e84ee3c8539df3da4713a08867649ced0d88dcbc Mon Sep 17 00:00:00 2001 From: Manon Deloupy Date: Tue, 11 Jun 2019 14:48:30 -0400 Subject: [PATCH 2/4] Styling offenses --- app/models/shipit/stack.rb | 2 +- dump.rdb | Bin 0 -> 92 bytes test/controllers/api/locks_controller_test.rb | 4 ++-- 3 files changed, 3 insertions(+), 3 deletions(-) create mode 100644 dump.rdb diff --git a/app/models/shipit/stack.rb b/app/models/shipit/stack.rb index 1629ee4ee..fe24b0163 100644 --- a/app/models/shipit/stack.rb +++ b/app/models/shipit/stack.rb @@ -398,7 +398,7 @@ def soft_locked? def lock(reason, user, lock_level: 'enforced') lock_level ||= 'enforced' - params = { lock_reason: reason, lock_author: user, lock_level: lock_level } + params = {lock_reason: reason, lock_author: user, lock_level: lock_level} update!(params) end diff --git a/dump.rdb b/dump.rdb new file mode 100644 index 0000000000000000000000000000000000000000..c39a0419dc7bad7bc96b02f80b15d3af516374b2 GIT binary patch literal 92 zcmWG?b@2=~Ffg$E#aWb^l3A=H&uTcA>dTGh$C%1OQ%ECL{m= literal 0 HcmV?d00001 diff --git a/test/controllers/api/locks_controller_test.rb b/test/controllers/api/locks_controller_test.rb index 804e53a5c..ada7c8e1f 100644 --- a/test/controllers/api/locks_controller_test.rb +++ b/test/controllers/api/locks_controller_test.rb @@ -18,7 +18,7 @@ class LocksControllerTest < ActionController::TestCase end test "#create sets a lock message if lock_level is advisory" do - post :create, params: { stack_id: @stack.to_param, reason: 'Just for fun!', lock_level: 'advisory' } + post :create, params: {stack_id: @stack.to_param, reason: 'Just for fun!', lock_level: 'advisory'} assert_response :ok assert_json 'is_locked', false assert_json 'lock_reason', 'Just for fun!' @@ -50,7 +50,7 @@ class LocksControllerTest < ActionController::TestCase end test "#update sets a lock with a lock_level if passed as a param" do - put :update, params: { stack_id: @stack.to_param, reason: 'Just for fun!', lock_level: 'advisory' } + put :update, params: {stack_id: @stack.to_param, reason: 'Just for fun!', lock_level: 'advisory'} assert_response :ok assert_json 'is_locked', false assert_json 'lock_reason', 'Just for fun!' From f3fc05aa7072529e18e7310c95ca956a527b650c Mon Sep 17 00:00:00 2001 From: Manon Deloupy Date: Tue, 11 Jun 2019 16:46:55 -0400 Subject: [PATCH 3/4] Addparams validation & use constants --- app/controllers/shipit/api/locks_controller.rb | 4 ++++ app/models/shipit/stack.rb | 10 ++++++---- dump.rdb | Bin 92 -> 193 bytes 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/controllers/shipit/api/locks_controller.rb b/app/controllers/shipit/api/locks_controller.rb index 472ee4d2f..20cdaedad 100644 --- a/app/controllers/shipit/api/locks_controller.rb +++ b/app/controllers/shipit/api/locks_controller.rb @@ -6,6 +6,8 @@ class LocksController < BaseController params do requires :reason, String, presence: true accepts :lock_level, String + + validates :lock_level, inclusion: { in: Shipit::Stack::LOCK_LEVELS } end def create if stack.locked? @@ -19,6 +21,8 @@ def create params do requires :reason, String, presence: true accepts :lock_level, String + + validates :lock_level, inclusion: { in: Shipit::Stack::LOCK_LEVELS } end def update stack.lock(params.reason, current_user, lock_level: params.lock_level) diff --git a/app/models/shipit/stack.rb b/app/models/shipit/stack.rb index fe24b0163..d15c87c42 100644 --- a/app/models/shipit/stack.rb +++ b/app/models/shipit/stack.rb @@ -23,7 +23,9 @@ def blank? ENVIRONMENT_MAX_SIZE = 50 REQUIRED_HOOKS = %i(push status).freeze - LOCK_LEVELS = %w(enforced advisory).freeze + LOCK_LEVEL_ENFORCED = "enforced".freeze + LOCK_LEVEL_ADVISORY = "advisory".freeze + LOCK_LEVELS = [LOCK_LEVEL_ENFORCED, LOCK_LEVEL_ADVISORY].freeze enum lock_level: LOCK_LEVELS.zip(LOCK_LEVELS).to_h has_many :commits, dependent: :destroy @@ -389,15 +391,15 @@ def active_task end def locked? - lock_reason.present? && lock_level != 'advisory' + lock_reason.present? && lock_level != LOCK_LEVEL_ADVISORY end def soft_locked? lock_reason.present? end - def lock(reason, user, lock_level: 'enforced') - lock_level ||= 'enforced' + def lock(reason, user, lock_level: LOCK_LEVEL_ENFORCED) + lock_level ||= LOCK_LEVEL_ENFORCED params = {lock_reason: reason, lock_author: user, lock_level: lock_level} update!(params) end diff --git a/dump.rdb b/dump.rdb index c39a0419dc7bad7bc96b02f80b15d3af516374b2..aeca318c3b974b511e192b2d1ff924ccad988c40 100644 GIT binary patch delta 152 zcmaz!$T-2E-dT_#_7_KKacYWgZffqK4UhR5e(@ycr|A|Hr6%Sk<)j{9_{Z>@k%7U% zH#j4+AhX2E$~UzrJvE>-CnqSiur#%}q}VGzNw-=lJ~1mkJtsdYF()%c$<|IOJu}5h vU%wcvNgt*+xFj(-Ti;Nrmfz6S%)rP($kf!(LeI>=(&WE%@?qs)d0q1W!i_dZ delta 50 zcmX@e7&E~@NA>&vm|q;F#i=Q}xv9B_E Date: Tue, 11 Jun 2019 17:33:12 -0400 Subject: [PATCH 4/4] Add test & make validation facultative --- app/controllers/shipit/api/locks_controller.rb | 4 ++-- test/controllers/api/locks_controller_test.rb | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/controllers/shipit/api/locks_controller.rb b/app/controllers/shipit/api/locks_controller.rb index 20cdaedad..1732e8c0e 100644 --- a/app/controllers/shipit/api/locks_controller.rb +++ b/app/controllers/shipit/api/locks_controller.rb @@ -7,7 +7,7 @@ class LocksController < BaseController requires :reason, String, presence: true accepts :lock_level, String - validates :lock_level, inclusion: { in: Shipit::Stack::LOCK_LEVELS } + validates :lock_level, inclusion: { in: Shipit::Stack::LOCK_LEVELS }, allow_nil: true end def create if stack.locked? @@ -22,7 +22,7 @@ def create requires :reason, String, presence: true accepts :lock_level, String - validates :lock_level, inclusion: { in: Shipit::Stack::LOCK_LEVELS } + validates :lock_level, inclusion: { in: Shipit::Stack::LOCK_LEVELS }, allow_nil: true end def update stack.lock(params.reason, current_user, lock_level: params.lock_level) diff --git a/test/controllers/api/locks_controller_test.rb b/test/controllers/api/locks_controller_test.rb index ada7c8e1f..ac0de9254 100644 --- a/test/controllers/api/locks_controller_test.rb +++ b/test/controllers/api/locks_controller_test.rb @@ -32,6 +32,12 @@ class LocksControllerTest < ActionController::TestCase assert_response :conflict end + test "#create fails if the lock_level is not included in the enum" do + post :create, params: {stack_id: @stack.to_param, reason: 'Just for fun!', lock_level: 'invalid'} + assert_response :unprocessable_entity + assert_json 'errors', 'lock_level' => ["is not included in the list"] + end + test "#update sets a lock" do put :update, params: {stack_id: @stack.to_param, reason: 'Just for fun!'} assert_response :ok