Merge branch 'ce-39118-dynamic-pipeline-variables-fe' into 'master'

Dynamic CI secret variables -- CE backport

See merge request gitlab-org/gitlab-ce!16842

26 files changed, 289 insertions, 197 deletions

diff --git a/app/assets/javascripts/ci_variable_list/ajax_variable_list.js b/app/assets/javascripts/ci_variable_list/ajax_variable_list.js
new file mode 100644
index 00000000000..76f93e5c6bd
--- /dev/null
+++ b/app/assets/javascripts/ci_variable_list/ajax_variable_list.js
@@ -0,0 +1,116 @@
+import _ from 'underscore';
+import axios from '../lib/utils/axios_utils';
+import { s__ } from '../locale';
+import Flash from '../flash';
+import { convertPermissionToBoolean } from '../lib/utils/common_utils';
+import statusCodes from '../lib/utils/http_status';
+import VariableList from './ci_variable_list';
+
+function generateErrorBoxContent(errors) {
+  const errorList = [].concat(errors).map(errorString => `
+    <li>
+      ${_.escape(errorString)}
+    </li>
+  `);
+
+  return `
+    <p>
+      ${s__('CiVariable|Validation failed')}
+    </p>
+    <ul>
+      ${errorList.join('')}
+    </ul>
+  `;
+}
+
+// Used for the variable list on CI/CD projects/groups settings page
+export default class AjaxVariableList {
+  constructor({
+    container,
+    saveButton,
+    errorBox,
+    formField = 'variables',
+    saveEndpoint,
+  }) {
+    this.container = container;
+    this.saveButton = saveButton;
+    this.errorBox = errorBox;
+    this.saveEndpoint = saveEndpoint;
+
+    this.variableList = new VariableList({
+      container: this.container,
+      formField,
+    });
+
+    this.bindEvents();
+    this.variableList.init();
+  }
+
+  bindEvents() {
+    this.saveButton.addEventListener('click', this.onSaveClicked.bind(this));
+  }
+
+  onSaveClicked() {
+    const loadingIcon = this.saveButton.querySelector('.js-secret-variables-save-loading-icon');
+    loadingIcon.classList.toggle('hide', false);
+    this.errorBox.classList.toggle('hide', true);
+    // We use this to prevent a user from changing a key before we have a chance
+    // to match it up in `updateRowsWithPersistedVariables`
+    this.variableList.toggleEnableRow(false);
+
+    return axios.patch(this.saveEndpoint, {
+      variables_attributes: this.variableList.getAllData(),
+    }, {
+      // We want to be able to process the `res.data` from a 400 error response
+      // and print the validation messages such as duplicate variable keys
+      validateStatus: status => (
+          status >= statusCodes.OK &&
+          status < statusCodes.MULTIPLE_CHOICES
+        ) ||
+        status === statusCodes.BAD_REQUEST,
+    })
+      .then((res) => {
+        loadingIcon.classList.toggle('hide', true);
+        this.variableList.toggleEnableRow(true);
+
+        if (res.status === statusCodes.OK && res.data) {
+          this.updateRowsWithPersistedVariables(res.data.variables);
+        } else if (res.status === statusCodes.BAD_REQUEST) {
+          // Validation failed
+          this.errorBox.innerHTML = generateErrorBoxContent(res.data);
+          this.errorBox.classList.toggle('hide', false);
+        }
+      })
+      .catch(() => {
+        loadingIcon.classList.toggle('hide', true);
+        this.variableList.toggleEnableRow(true);
+        Flash(s__('CiVariable|Error occured while saving variables'));
+      });
+  }
+
+  updateRowsWithPersistedVariables(persistedVariables = []) {
+    const persistedVariableMap = [].concat(persistedVariables).reduce((variableMap, variable) => ({
+      ...variableMap,
+      [variable.key]: variable,
+    }), {});
+
+    this.container.querySelectorAll('.js-row').forEach((row) => {
+      // If we submitted a row that was destroyed, remove it so we don't try
+      // to destroy it again which would cause a BE error
+      const destroyInput = row.querySelector('.js-ci-variable-input-destroy');
+      if (convertPermissionToBoolean(destroyInput.value)) {
+        row.remove();
+      // Update the ID input so any future edits and `_destroy` will apply on the BE
+      } else {
+        const key = row.querySelector('.js-ci-variable-input-key').value;
+        const persistedVariable = persistedVariableMap[key];
+
+        if (persistedVariable) {
+          // eslint-disable-next-line no-param-reassign
+          row.querySelector('.js-ci-variable-input-id').value = persistedVariable.id;
+          row.setAttribute('data-is-persisted', 'true');
+        }
+      }
+    });
+  }
+}
diff --git a/app/assets/javascripts/ci_variable_list/ci_variable_list.js b/app/assets/javascripts/ci_variable_list/ci_variable_list.js
index e46478ddb98..d91789c2192 100644
--- a/app/assets/javascripts/ci_variable_list/ci_variable_list.js
+++ b/app/assets/javascripts/ci_variable_list/ci_variable_list.js
@@ -11,7 +11,7 @@ function createEnvironmentItem(value) {
   return {
     title: value === '*' ? ALL_ENVIRONMENTS_STRING : value,
     id: value,
-    text: value,
+    text: value === '*' ? s__('CiVariable|* (All environments)') : value,
   };
 }
 
@@ -41,11 +41,11 @@ export default class VariableList {
         selector: '.js-ci-variable-input-protected',
         default: 'true',
       },
-      environment: {
+      environment_scope: {
         // We can't use a `.js-` class here because
         // gl_dropdown replaces the <input> and doesn't copy over the class
         // See https://gitlab.com/gitlab-org/gitlab-ce/issues/42458
-        selector: `input[name="${this.formField}[variables_attributes][][environment]"]`,
+        selector: `input[name="${this.formField}[variables_attributes][][environment_scope]"]`,
         default: '*',
       },
       _destroy: {
@@ -104,12 +104,15 @@ export default class VariableList {
 
     setupToggleButtons($row[0]);
 
+    // Reset the resizable textarea
+    $row.find(this.inputMap.value.selector).css('height', '');
+
     const $environmentSelect = $row.find('.js-variable-environment-toggle');
     if ($environmentSelect.length) {
       const createItemDropdown = new CreateItemDropdown({
         $dropdown: $environmentSelect,
         defaultToggleLabel: ALL_ENVIRONMENTS_STRING,
-        fieldName: `${this.formField}[variables_attributes][][environment]`,
+        fieldName: `${this.formField}[variables_attributes][][environment_scope]`,
         getData: (term, callback) => callback(this.getEnvironmentValues()),
         createNewItemFromValue: createEnvironmentItem,
         onSelect: () => {
@@ -117,7 +120,7 @@ export default class VariableList {
           // so they have the new value we just picked
           this.refreshDropdownData();
 
-          $row.find(this.inputMap.environment.selector).trigger('trigger-change');
+          $row.find(this.inputMap.environment_scope.selector).trigger('trigger-change');
         },
       });
 
@@ -143,7 +146,8 @@ export default class VariableList {
     $row.after($rowClone);
   }
 
-  removeRow($row) {
+  removeRow(row) {
+    const $row = $(row);
     const isPersisted = convertPermissionToBoolean($row.attr('data-is-persisted'));
 
     if (isPersisted) {
@@ -155,6 +159,10 @@ export default class VariableList {
     } else {
       $row.remove();
     }
+
+    // Refresh the other dropdowns in the variable list
+    // so any value with the variable deleted is gone
+    this.refreshDropdownData();
   }
 
   checkIfRowTouched($row) {
@@ -165,6 +173,11 @@ export default class VariableList {
     });
   }
 
+  toggleEnableRow(isEnabled = true) {
+    this.$container.find(this.inputMap.key.selector).attr('disabled', !isEnabled);
+    this.$container.find('.js-row-remove-button').attr('disabled', !isEnabled);
+  }
+
   getAllData() {
     // Ignore the last empty row because we don't want to try persist
     // a blank variable and run into validation problems.
@@ -185,7 +198,7 @@ export default class VariableList {
   }
 
   getEnvironmentValues() {
-    const valueMap = this.$container.find(this.inputMap.environment.selector).toArray()
+    const valueMap = this.$container.find(this.inputMap.environment_scope.selector).toArray()
       .reduce((prevValueMap, envInput) => ({
         ...prevValueMap,
         [envInput.value]: envInput.value,
diff --git a/app/assets/javascripts/commons/polyfills/element.js b/app/assets/javascripts/commons/polyfills/element.js
index 9a1f73bf2ac..b593bde6aa2 100644
--- a/app/assets/javascripts/commons/polyfills/element.js
+++ b/app/assets/javascripts/commons/polyfills/element.js
@@ -18,3 +18,22 @@ Element.prototype.matches = Element.prototype.matches ||
     while (i >= 0 && elms.item(i) !== this) { i -= 1; }
     return i > -1;
   };
+
+// From the polyfill on MDN, https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/remove#Polyfill
+((arr) => {
+  arr.forEach((item) => {
+    if (Object.prototype.hasOwnProperty.call(item, 'remove')) {
+      return;
+    }
+    Object.defineProperty(item, 'remove', {
+      configurable: true,
+      enumerable: true,
+      writable: true,
+      value: function remove() {
+        if (this.parentNode !== null) {
+          this.parentNode.removeChild(this);
+        }
+      },
+    });
+  });
+})([Element.prototype, CharacterData.prototype, DocumentType.prototype]);
diff --git a/app/assets/javascripts/lib/utils/http_status.js b/app/assets/javascripts/lib/utils/http_status.js
index 625e53ee9de..bb151929431 100644
--- a/app/assets/javascripts/lib/utils/http_status.js
+++ b/app/assets/javascripts/lib/utils/http_status.js
@@ -6,4 +6,6 @@ export default {
   ABORTED: 0,
   NO_CONTENT: 204,
   OK: 200,
+  MULTIPLE_CHOICES: 300,
+  BAD_REQUEST: 400,
 };
diff --git a/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js b/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js
index f26c7360fbe..ad79f7e09ac 100644
--- a/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js
+++ b/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js
@@ -1,11 +1,12 @@
-import SecretValues from '~/behaviors/secret_values';
+import AjaxVariableList from '~/ci_variable_list/ajax_variable_list';
 
 export default () => {
-  const secretVariableTable = document.querySelector('.js-secret-variable-table');
-  if (secretVariableTable) {
-    const secretVariableTableValues = new SecretValues({
-      container: secretVariableTable,
-    });
-    secretVariableTableValues.init();
-  }
+  const variableListEl = document.querySelector('.js-ci-variable-list-section');
+  // eslint-disable-next-line no-new
+  new AjaxVariableList({
+    container: variableListEl,
+    saveButton: variableListEl.querySelector('.js-secret-variables-save-button'),
+    errorBox: variableListEl.querySelector('.js-ci-variable-error-box'),
+    saveEndpoint: variableListEl.dataset.saveEndpoint,
+  });
 };
diff --git a/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js b/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js
index 18dc1dc03a5..a563d0f9961 100644
--- a/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js
+++ b/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js
@@ -1,9 +1,11 @@
 import initSettingsPanels from '~/settings_panels';
 import SecretValues from '~/behaviors/secret_values';
+import AjaxVariableList from '~/ci_variable_list/ajax_variable_list';
 
 export default function () {
   // Initialize expandable settings panels
   initSettingsPanels();
+
   const runnerToken = document.querySelector('.js-secret-runner-token');
   if (runnerToken) {
     const runnerTokenSecretValue = new SecretValues({
@@ -12,11 +14,12 @@ export default function () {
     runnerTokenSecretValue.init();
   }
 
-  const secretVariableTable = document.querySelector('.js-secret-variable-table');
-  if (secretVariableTable) {
-    const secretVariableTableValues = new SecretValues({
-      container: secretVariableTable,
-    });
-    secretVariableTableValues.init();
-  }
+  const variableListEl = document.querySelector('.js-ci-variable-list-section');
+  // eslint-disable-next-line no-new
+  new AjaxVariableList({
+    container: variableListEl,
+    saveButton: variableListEl.querySelector('.js-secret-variables-save-button'),
+    errorBox: variableListEl.querySelector('.js-ci-variable-error-box'),
+    saveEndpoint: variableListEl.dataset.saveEndpoint,
+  });
 }
diff --git a/app/assets/stylesheets/framework/ci_variable_list.scss b/app/assets/stylesheets/framework/ci_variable_list.scss
index 8f654ab363c..5fe835dd8f9 100644
--- a/app/assets/stylesheets/framework/ci_variable_list.scss
+++ b/app/assets/stylesheets/framework/ci_variable_list.scss
@@ -8,7 +8,11 @@
 
 .ci-variable-row {
   display: flex;
-  align-items: flex-end;
+  align-items: flex-start;
+
+  @media (max-width: $screen-xs-max) {
+    align-items: flex-end;
+  }
 
   &:not(:last-child) {
     margin-bottom: $gl-btn-padding;
@@ -41,6 +45,7 @@
 
 .ci-variable-row-body {
   display: flex;
+  align-items: flex-start;
   width: 100%;
 
   @media (max-width: $screen-xs-max) {
@@ -65,6 +70,8 @@
   flex: 0 1 auto;
   display: flex;
   align-items: center;
+  padding-top: 5px;
+  padding-bottom: 5px;
 }
 
 .ci-variable-row-remove-button {
@@ -85,4 +92,8 @@
     outline: none;
     color: $gl-text-color;
   }
+
+  &[disabled] {
+    color: $gl-text-color-disabled;
+  }
 }
diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb
index 10038ff3ad9..913e13bf734 100644
--- a/app/controllers/groups/variables_controller.rb
+++ b/app/controllers/groups/variables_controller.rb
@@ -1,60 +1,43 @@
 module Groups
   class VariablesController < Groups::ApplicationController
-    before_action :variable, only: [:show, :update, :destroy]
     before_action :authorize_admin_build!
 
-    def index
-      redirect_to group_settings_ci_cd_path(group)
-    end
-
     def show
+      respond_to do |format|
+        format.json do
+          render status: :ok, json: { variables: GroupVariableSerializer.new.represent(@group.variables) }
+        end
+      end
     end
 
     def update
-      if variable.update(variable_params)
-        redirect_to group_variables_path(group),
-                    notice: 'Variable was successfully updated.'
+      if @group.update(group_variables_params)
+        respond_to do |format|
+          format.json { return render_group_variables }
+        end
       else
-        render "show"
+        respond_to do |format|
+          format.json { render_error }
+        end
       end
     end
 
-    def create
-      @variable = group.variables.create(variable_params)
-        .present(current_user: current_user)
+    private
 
-      if @variable.persisted?
-        redirect_to group_settings_ci_cd_path(group),
-                    notice: 'Variable was successfully created.'
-      else
-        render "show"
-      end
+    def render_group_variables
+      render status: :ok, json: { variables: GroupVariableSerializer.new.represent(@group.variables) }
     end
 
-    def destroy
-      if variable.destroy
-        redirect_to group_settings_ci_cd_path(group),
-                    status: 302,
-                    notice: 'Variable was successfully removed.'
-      else
-        redirect_to group_settings_ci_cd_path(group),
-                    status: 302,
-                    notice: 'Failed to remove the variable.'
-      end
+    def render_error
+      render status: :bad_request, json: @group.errors.full_messages
     end
 
-    private
-
-    def variable_params
-      params.require(:variable).permit(*variable_params_attributes)
+    def group_variables_params
+      params.permit(variables_attributes: [*variable_params_attributes])
     end
 
     def variable_params_attributes
-      %i[key value protected]
-    end
-
-    def variable
-      @variable ||= group.variables.find(params[:id]).present(current_user: current_user)
+      %i[id key value protected _destroy]
     end
 
     def authorize_admin_build!
diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb
index 6a825137564..7eb509e2e64 100644
--- a/app/controllers/projects/variables_controller.rb
+++ b/app/controllers/projects/variables_controller.rb
@@ -1,60 +1,41 @@
 class Projects::VariablesController < Projects::ApplicationController
-  before_action :variable, only: [:show, :update, :destroy]
   before_action :authorize_admin_build!
 
-  layout 'project_settings'
-
-  def index
-    redirect_to project_settings_ci_cd_path(@project)
-  end
-
   def show
+    respond_to do |format|
+      format.json do
+        render status: :ok, json: { variables: VariableSerializer.new.represent(@project.variables) }
+      end
+    end
   end
 
   def update
-    if variable.update(variable_params)
-      redirect_to project_variables_path(project),
-                  notice: 'Variable was successfully updated.'
+    if @project.update(variables_params)
+      respond_to do |format|
+        format.json { return render_variables }
+      end
     else
-      render "show"
+      respond_to do |format|
+        format.json { render_error }
+      end
     end
   end
 
-  def create
-    @variable = project.variables.create(variable_params)
-      .present(current_user: current_user)
+  private
 
-    if @variable.persisted?
-      redirect_to project_settings_ci_cd_path(project),
-                  notice: 'Variable was successfully created.'
-    else
-      render "show"
-    end
+  def render_variables
+    render status: :ok, json: { variables: VariableSerializer.new.represent(@project.variables) }
   end
 
-  def destroy
-    if variable.destroy
-      redirect_to project_settings_ci_cd_path(project),
-                  status: 302,
-                  notice: 'Variable was successfully removed.'
-    else
-      redirect_to project_settings_ci_cd_path(project),
-                  status: 302,
-                  notice: 'Failed to remove the variable.'
-    end
+  def render_error
+    render status: :bad_request, json: @project.errors.full_messages
   end
 
-  private
-
-  def variable_params
-    params.require(:variable).permit(*variable_params_attributes)
+  def variables_params
+    params.permit(variables_attributes: [*variable_params_attributes])
   end
 
   def variable_params_attributes
     %i[id key value protected _destroy]
   end
-
-  def variable
-    @variable ||= project.variables.find(params[:id]).present(current_user: current_user)
-  end
 end
diff --git a/app/models/group.rb b/app/models/group.rb
index 5b7f1b38612..75bf013ecd2 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -31,9 +31,12 @@ class Group < Namespace
 
   has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
+  accepts_nested_attributes_for :variables, allow_destroy: true
+
   validate :visibility_level_allowed_by_projects
   validate :visibility_level_allowed_by_sub_groups
   validate :visibility_level_allowed_by_parent
+  validates :variables, variable_duplicates: true
 
   validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 }
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 3edb6109fb8..34fb39dd28e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -260,6 +260,7 @@ class Project < ActiveRecord::Base
   validates :repository_storage,
     presence: true,
     inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
+  validates :variables, variable_duplicates: true
 
   has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
diff --git a/app/presenters/ci/group_variable_presenter.rb b/app/presenters/ci/group_variable_presenter.rb
index 81fea106a5c..98d68bc7a83 100644
--- a/app/presenters/ci/group_variable_presenter.rb
+++ b/app/presenters/ci/group_variable_presenter.rb
@@ -7,19 +7,15 @@ module Ci
     end
 
     def form_path
-      if variable.persisted?
-        group_variable_path(group, variable)
-      else
-        group_variables_path(group)
-      end
+      group_settings_ci_cd_path(group)
     end
 
     def edit_path
-      group_variable_path(group, variable)
+      group_variables_path(group)
     end
 
     def delete_path
-      group_variable_path(group, variable)
+      group_variables_path(group)
     end
   end
 end
diff --git a/app/presenters/ci/variable_presenter.rb b/app/presenters/ci/variable_presenter.rb
index 5d7998393a6..96159f88c59 100644
--- a/app/presenters/ci/variable_presenter.rb
+++ b/app/presenters/ci/variable_presenter.rb
@@ -7,19 +7,15 @@ module Ci
     end
 
     def form_path
-      if variable.persisted?
-        project_variable_path(project, variable)
-      else
-        project_variables_path(project)
-      end
+      project_settings_ci_cd_path(project)
     end
 
     def edit_path
-      project_variable_path(project, variable)
+      project_variables_path(project)
     end
 
     def delete_path
-      project_variable_path(project, variable)
+      project_variables_path(project)
     end
   end
 end
diff --git a/app/serializers/group_variable_entity.rb b/app/serializers/group_variable_entity.rb
new file mode 100644
index 00000000000..62cf0b21e1e
--- /dev/null
+++ b/app/serializers/group_variable_entity.rb
@@ -0,0 +1,7 @@
+class GroupVariableEntity < Grape::Entity
+  expose :id
+  expose :key
+  expose :value
+
+  expose :protected?, as: :protected
+end
diff --git a/app/serializers/group_variable_serializer.rb b/app/serializers/group_variable_serializer.rb
new file mode 100644
index 00000000000..8f8205924aa
--- /dev/null
+++ b/app/serializers/group_variable_serializer.rb
@@ -0,0 +1,3 @@
+class GroupVariableSerializer < BaseSerializer
+  entity GroupVariableEntity
+end
diff --git a/app/serializers/variable_entity.rb b/app/serializers/variable_entity.rb
new file mode 100644
index 00000000000..d576745c073
--- /dev/null
+++ b/app/serializers/variable_entity.rb
@@ -0,0 +1,7 @@
+class VariableEntity < Grape::Entity
+  expose :id
+  expose :key
+  expose :value
+
+  expose :protected?, as: :protected
+end
diff --git a/app/serializers/variable_serializer.rb b/app/serializers/variable_serializer.rb
new file mode 100644
index 00000000000..32ae82ab51c
--- /dev/null
+++ b/app/serializers/variable_serializer.rb
@@ -0,0 +1,3 @@
+class VariableSerializer < BaseSerializer
+  entity VariableEntity
+end
diff --git a/app/views/ci/variables/_content.html.haml b/app/views/ci/variables/_content.html.haml
index fbfe3e56588..d355e7799df 100644
--- a/app/views/ci/variables/_content.html.haml
+++ b/app/views/ci/variables/_content.html.haml
@@ -1,3 +1 @@
-%p.append-bottom-default
-  Variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags.
-  You can use variables for passwords, secret keys, or whatever you want.
+= _('Variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. You can use variables for passwords, secret keys, or whatever you want.')
diff --git a/app/views/ci/variables/_form.html.haml b/app/views/ci/variables/_form.html.haml
deleted file mode 100644
index eebd0955c80..00000000000
--- a/app/views/ci/variables/_form.html.haml
+++ /dev/null
@@ -1,19 +0,0 @@
-= form_for @variable, as: :variable, url: @variable.form_path do |f|
-  = form_errors(@variable)
-
-  .form-group
-    = f.label :key, "Key", class: "label-light"
-    = f.text_field :key, class: "form-control", placeholder: @variable.placeholder, required: true
-  .form-group
-    = f.label :value, "Value", class: "label-light"
-    = f.text_area :value, class: "form-control", placeholder: @variable.placeholder
-  .form-group
-    .checkbox
-      = f.label :protected do
-        = f.check_box :protected
-        %strong Protected
-      .help-block
-        This variable will be passed only to pipelines running on protected branches and tags
-        = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'protected-secret-variables'), target: '_blank'
-
-  = f.submit btn_text, class: "btn btn-save"
diff --git a/app/views/ci/variables/_index.html.haml b/app/views/ci/variables/_index.html.haml
index 6e399fc7392..e402801a776 100644
--- a/app/views/ci/variables/_index.html.haml
+++ b/app/views/ci/variables/_index.html.haml
@@ -1,16 +1,20 @@
-.row.prepend-top-default.append-bottom-default
-  .col-lg-12
-    %h5.prepend-top-0
-      Add a variable
-    = render "ci/variables/form", btn_text: "Add new variable"
-    %hr
-    %h5.prepend-top-0
-      Your variables (#{@variables.size})
-    - if @variables.empty?
-      %p.settings-message.text-center.append-bottom-0
-        No variables found, add one with the form above.
-    - else
-      .js-secret-variable-table
-        = render "ci/variables/table"
-        %button.btn.btn-info.js-secret-value-reveal-button{ data: { secret_reveal_status: 'false' } }
+- save_endpoint = local_assigns.fetch(:save_endpoint, nil)
+
+.row
+  .col-lg-12.js-ci-variable-list-section{ data: { save_endpoint: save_endpoint } }
+    .hide.alert.alert-danger.js-ci-variable-error-box
+
+    %ul.ci-variable-list
+      - @variables.each.each do |variable|
+        = render 'ci/variables/variable_row', form_field: 'variables', variable: variable
+      = render 'ci/variables/variable_row', form_field: 'variables'
+    .prepend-top-20
+      %button.btn.btn-success.js-secret-variables-save-button{ type: 'button' }
+        %span.hide.js-secret-variables-save-loading-icon
+          = icon('spinner spin')
+        = _('Save variables')
+      %button.btn.btn-info.btn-inverted.prepend-left-10.js-secret-value-reveal-button{ type: 'button', data: { secret_reveal_status: "#{@variables.size == 0}" } }
+        - if @variables.size == 0
+          = n_('Hide value', 'Hide values', @variables.size)
+        - else
           = n_('Reveal value', 'Reveal values', @variables.size)
diff --git a/app/views/ci/variables/_show.html.haml b/app/views/ci/variables/_show.html.haml
deleted file mode 100644
index 6d75ae96124..00000000000
--- a/app/views/ci/variables/_show.html.haml
+++ /dev/null
@@ -1,9 +0,0 @@
-- page_title "Variables"
-
-.row.prepend-top-default.append-bottom-default
-  .col-lg-3
-    = render "ci/variables/content"
-  .col-lg-9
-    %h4.prepend-top-0
-      Update variable
-    = render "ci/variables/form", btn_text: "Save variable"
diff --git a/app/views/ci/variables/_table.html.haml b/app/views/ci/variables/_table.html.haml
deleted file mode 100644
index 2298930d0c7..00000000000
--- a/app/views/ci/variables/_table.html.haml
+++ /dev/null
@@ -1,32 +0,0 @@
-.table-responsive.variables-table
-  %table.table
-    %colgroup
-      %col
-      %col
-      %col
-      %col{ width: 100 }
-    %thead
-      %th Key
-      %th Value
-      %th Protected
-      %th
-    %tbody
-      - @variables.each do |variable|
-        - if variable.id?
-          %tr
-            %td.variable-key= variable.key
-            %td.variable-value
-              %span.js-secret-value-placeholder
-                = '*' * 6
-              %span.hide.js-secret-value
-                = variable.value
-            %td.variable-protected= Gitlab::Utils.boolean_to_yes_no(variable.protected)
-            %td.variable-menu
-              = link_to variable.edit_path, class: "btn btn-transparent btn-variable-edit" do
-                %span.sr-only
-                  Update
-                = icon("pencil")
-              = link_to variable.delete_path, class: "btn btn-transparent btn-variable-delete", method: :delete, data: { confirm: "Are you sure?" } do
-                %span.sr-only
-                  Remove
-                = icon("trash")
diff --git a/app/views/groups/settings/ci_cd/show.html.haml b/app/views/groups/settings/ci_cd/show.html.haml
index 472da2a6a72..dd82922ec55 100644
--- a/app/views/groups/settings/ci_cd/show.html.haml
+++ b/app/views/groups/settings/ci_cd/show.html.haml
@@ -1,4 +1,11 @@
 - breadcrumb_title "CI / CD Settings"
 - page_title "CI / CD"
 
-= render 'ci/variables/index'
+%h4
+  = _('Secret variables')
+  = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'secret-variables'), target: '_blank', rel: 'noopener noreferrer'
+
+%p
+  = render "ci/variables/content"
+
+= render 'ci/variables/index', save_endpoint: group_variables_path
diff --git a/app/views/groups/variables/show.html.haml b/app/views/groups/variables/show.html.haml
deleted file mode 100644
index df533952b76..00000000000
--- a/app/views/groups/variables/show.html.haml
+++ /dev/null
@@ -1 +0,0 @@
-= render 'ci/variables/show'
diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml
index 664a4554692..756f31f91d9 100644
--- a/app/views/projects/settings/ci_cd/show.html.haml
+++ b/app/views/projects/settings/ci_cd/show.html.haml
@@ -29,14 +29,14 @@
 %section.settings.no-animate{ class: ('expanded' if expanded) }
   .settings-header
     %h4
-      Secret variables
-      = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'secret-variables'), target: '_blank'
+      = _('Secret variables')
+      = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'secret-variables'), target: '_blank', rel: 'noopener noreferrer'
     %button.btn.js-settings-toggle
       = expanded ? 'Collapse' : 'Expand'
-    %p
+    %p.append-bottom-0
       = render "ci/variables/content"
   .settings-content
-    = render 'ci/variables/index'
+    = render 'ci/variables/index', save_endpoint: project_variables_path(@project)
 
 %section.settings.no-animate{ class: ('expanded' if expanded) }
   .settings-header
diff --git a/app/views/projects/variables/show.html.haml b/app/views/projects/variables/show.html.haml
deleted file mode 100644
index df533952b76..00000000000
--- a/app/views/projects/variables/show.html.haml
+++ /dev/null
@@ -1 +0,0 @@
-= render 'ci/variables/show'