Update FE JobLog feature to always be true

This is to help the API remove the feature flag
for job log with minimal FE changes. Set the
`isNewJobLog` to always return true
and removes the unneccessary tests.

app/assets/javascripts/jobs/components/job_app.vue

@@ -17,7 +17,7 @@ import UnmetPrerequisitesBlock from './unmet_prerequisites_block.vue';
 import Sidebar from './sidebar.vue';
 import { sprintf } from '~/locale';
 import delayedJobMixin from '../mixins/delayed_job_mixin';
-import { isNewJobLogActive } from '../store/utils';
+import Log from './log/log.vue';
 
 export default {
   name: 'JobPageApp',
@@ -28,7 +28,7 @@ export default {
     EnvironmentsBlock,
     ErasedBlock,
     Icon,
-    Log: () => (isNewJobLogActive() ? import('./log/log.vue') : import('./job_log.vue')),
+    Log,
     LogTopBar,
     StuckBlock,
     UnmetPrerequisitesBlock,

app/assets/javascripts/jobs/components/job_log.vue

-<script>
-import { mapState, mapActions } from 'vuex';
-
-export default {
-  name: 'JobLog',
-  props: {
-    trace: {
-      type: String,
-      required: true,
-    },
-    isComplete: {
-      type: Boolean,
-      required: true,
-    },
-  },
-  computed: {
-    ...mapState(['isScrolledToBottomBeforeReceivingTrace']),
-  },
-  updated() {
-    this.$nextTick(() => {
-      this.handleScrollDown();
-    });
-  },
-  mounted() {
-    this.$nextTick(() => {
-      this.handleScrollDown();
-    });
-  },
-  methods: {
-    ...mapActions(['scrollBottom']),
-    /**
-     * The job log is sent in HTML, which means we need to use `v-html` to render it
-     * Using the updated hook with $nextTick is not enough to wait for the DOM to be updated
-     * in this case because it runs before `v-html` has finished running, since there's no
-     * Vue binding.
-     * In order to scroll the page down after `v-html` has finished, we need to use setTimeout
-     */
-    handleScrollDown() {
-      if (this.isScrolledToBottomBeforeReceivingTrace) {
-        setTimeout(() => {
-          this.scrollBottom();
-        }, 0);
-      }
-    },
-  },
-};
-</script>
-<template>
-  <pre class="js-build-trace build-trace qa-build-trace">
-    <code class="bash" v-html="trace">
-    </code>
-
-    <div v-if="!isComplete" class="js-log-animation build-loader-animation">
-      <div class="dot"></div>
-      <div class="dot"></div>
-      <div class="dot"></div>
-    </div>
-  </pre>
-</template>

app/assets/javascripts/jobs/store/mutations.js

 import Vue from 'vue';
 import * as types from './mutation_types';
-import { logLinesParser, updateIncrementalTrace, isNewJobLogActive } from './utils';
+import { logLinesParser, updateIncrementalTrace } from './utils';
 
 export default {
   [types.SET_JOB_ENDPOINT](state, endpoint) {
@@ -25,22 +25,16 @@ export default {
     }
 
     if (log.append) {
-      if (isNewJobLogActive()) {
-        state.trace = log.lines ? updateIncrementalTrace(log.lines, state.trace) : state.trace;
-      } else {
-        state.trace += log.html;
-      }
+      state.trace = log.lines ? updateIncrementalTrace(log.lines, state.trace) : state.trace;
+
       state.traceSize += log.size;
     } else {
       // When the job still does not have a trace
       // the trace response will not have a defined
       // html or size. We keep the old value otherwise these
       // will be set to `null`
-      if (isNewJobLogActive()) {
-        state.trace = log.lines ? logLinesParser(log.lines) : state.trace;
-      } else {
-        state.trace = log.html || state.trace;
-      }
+      state.trace = log.lines ? logLinesParser(log.lines) : state.trace;
+
       state.traceSize = log.size || state.traceSize;
     }
 

app/assets/javascripts/jobs/store/state.js

-import { isNewJobLogActive } from './utils';
-
 export default () => ({
   jobEndpoint: null,
   traceEndpoint: null,
@@ -18,7 +16,7 @@ export default () => ({
   // Used to check if we should keep the automatic scroll
   isScrolledToBottomBeforeReceivingTrace: true,
 
-  trace: isNewJobLogActive() ? [] : '',
+  trace: [],
   isTraceComplete: false,
   traceSize: 0,
   isTraceSizeVisible: false,

app/assets/javascripts/jobs/store/utils.js

@@ -177,5 +177,3 @@ export const updateIncrementalTrace = (newLog = [], oldParsed = []) => {
 
   return logLinesParser(newLog, parsedLog);
 };
-
-export const isNewJobLogActive = () => gon && gon.features && gon.features.jobLogJson;

app/controllers/projects/jobs_controller.rb

@@ -11,9 +11,6 @@ class Projects::JobsController < Projects::ApplicationController
   before_action :authorize_erase_build!, only: [:erase]
   before_action :authorize_use_build_terminal!, only: [:terminal, :terminal_websocket_authorize]
   before_action :verify_api_request!, only: :terminal_websocket_authorize
-  before_action only: [:show] do
-    push_frontend_feature_flag(:job_log_json, project, default_enabled: true)
-  end
   before_action :authorize_create_proxy_build!, only: :proxy_websocket_authorize
   before_action :verify_proxy_request!, only: :proxy_websocket_authorize
 
@@ -55,15 +52,10 @@ class Projects::JobsController < Projects::ApplicationController
         format.json do
           build.trace.being_watched!
 
-          # TODO: when the feature flag is removed we should not pass
-          # content_format to serialize method.
-          content_format = Feature.enabled?(:job_log_json, @project, default_enabled: true) ? :json : :html
-
           build_trace = Ci::BuildTrace.new(
             build: @build,
             stream: stream,
-            state: params[:state],
-            content_format: content_format)
+            state: params[:state])
 
           render json: BuildTraceSerializer
             .new(project: @project, current_user: @current_user)

app/models/ci/build_trace.rb

@@ -2,40 +2,22 @@
 
 module Ci
   class BuildTrace
-    CONVERTERS = {
-      html: Gitlab::Ci::Ansi2html,
-      json: Gitlab::Ci::Ansi2json
-    }.freeze
-
     attr_reader :trace, :build
 
     delegate :state, :append, :truncated, :offset, :size, :total, to: :trace, allow_nil: true
     delegate :id, :status, :complete?, to: :build, prefix: true
 
-    def initialize(build:, stream:, state:, content_format:)
+    def initialize(build:, stream:, state:)
       @build = build
-      @content_format = content_format
 
       if stream.valid?
         stream.limit
-        @trace = CONVERTERS.fetch(content_format).convert(stream.stream, state)
+        @trace = Gitlab::Ci::Ansi2json.convert(stream.stream, state)
       end
     end
 
-    def json?
-      @content_format == :json
-    end
-
-    def html?
-      @content_format == :html
-    end
-
-    def json_lines
-      @trace&.lines if json?
-    end
-
-    def html_lines
-      @trace&.html if html?
+    def lines
+      @trace&.lines
     end
   end
 end

app/serializers/build_trace_entity.rb

@@ -12,6 +12,5 @@ class BuildTraceEntity < Grape::Entity
   expose :size
   expose :total
 
-  expose :json_lines, as: :lines, if: ->(*) { object.json? }
-  expose :html_lines, as: :html, if: ->(*) { object.html? }
+  expose :lines
 end

changelogs/unreleased/remove-ff-job-log-json.yml

+---
+title: Remove legacy job log rendering
+merge_request: 34538
+author:
+type: other

qa/qa/specs/features/ee/browser_ui/secure/create_merge_request_with_secure_spec.rb

@@ -13,8 +13,6 @@ module QA
 
       after do
         @runner.remove_via_api! if @runner
-
-        Runtime::Feature.enable('job_log_json') if @job_log_json_flag_enabled
       end
 
       before do

qa/qa/specs/features/ee/browser_ui/secure/vulnerability_management_spec.rb

@@ -13,8 +13,6 @@ module QA
 
       after(:all) do
         @runner.remove_via_api!
-
-        Runtime::Feature.enable('job_log_json') if @job_log_json_flag_enabled
       end
 
       before(:all) do

spec/controllers/projects/jobs_controller_spec.rb

@@ -646,109 +646,6 @@ RSpec.describe Projects::JobsController, :clean_gitlab_redis_shared_state do
     end
   end
 
-  describe 'GET legacy trace.json' do
-    before do
-      stub_feature_flags(job_log_json: false)
-      get_trace
-    end
-
-    context 'when job has a trace artifact' do
-      let(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) }
-
-      it 'returns a trace' do
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response['id']).to eq job.id
-        expect(json_response['status']).to eq job.status
-        expect(json_response['state']).to be_present
-        expect(json_response['append']).not_to be_nil
-        expect(json_response['truncated']).not_to be_nil
-        expect(json_response['size']).to be_present
-        expect(json_response['total']).to be_present
-        expect(json_response['html']).to eq(job.trace.html)
-      end
-    end
-
-    context 'when job has a trace' do
-      let(:job) { create(:ci_build, :trace_live, pipeline: pipeline) }
-
-      it 'returns a trace' do
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response['id']).to eq job.id
-        expect(json_response['status']).to eq job.status
-        expect(json_response['html']).to eq('<span>BUILD TRACE</span>')
-      end
-    end
-
-    context 'when job has no traces' do
-      let(:job) { create(:ci_build, pipeline: pipeline) }
-
-      it 'returns no traces' do
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response['id']).to eq job.id
-        expect(json_response['status']).to eq job.status
-        expect(json_response['html']).to be_nil
-      end
-    end
-
-    context 'when job has a trace with ANSI sequence and Unicode' do
-      let(:job) { create(:ci_build, :unicode_trace_live, pipeline: pipeline) }
-
-      it 'returns a trace with Unicode' do
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response['id']).to eq job.id
-        expect(json_response['status']).to eq job.status
-        expect(json_response['html']).to include("ヾ(´༎ຶД༎ຶ`)ﾉ")
-      end
-    end
-
-    context 'when trace artifact is in ObjectStorage' do
-      let(:url) { 'http://object-storage/trace' }
-      let(:file_path) { expand_fixture_path('trace/sample_trace') }
-      let!(:job) { create(:ci_build, :success, :trace_artifact, pipeline: pipeline) }
-
-      before do
-        allow_any_instance_of(JobArtifactUploader).to receive(:file_storage?) { false }
-        allow_any_instance_of(JobArtifactUploader).to receive(:url) { url }
-        allow_any_instance_of(JobArtifactUploader).to receive(:size) { File.size(file_path) }
-      end
-
-      context 'when there are no network issues' do
-        before do
-          stub_remote_url_206(url, file_path)
-
-          get_trace
-        end
-
-        it 'returns a trace' do
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response['id']).to eq job.id
-          expect(json_response['status']).to eq job.status
-          expect(json_response['html']).to eq(job.trace.html)
-        end
-      end
-
-      context 'when there is a network issue' do
-        before do
-          stub_remote_url_500(url)
-        end
-
-        it 'returns a trace' do
-          expect { get_trace }.to raise_error(Gitlab::HttpIO::FailedToGetChunkError)
-        end
-      end
-    end
-
-    def get_trace
-      get :trace,
-        params: {
-          namespace_id: project.namespace,
-          project_id: project,
-          id: job.id
-        },
-        format: :json
-    end
-  end
-
   describe 'GET status.json' do
     let(:job) { create(:ci_build, pipeline: pipeline) }
     let(:status) { job.detailed_status(double('user')) }

spec/features/projects/jobs_spec.rb

@@ -837,7 +837,7 @@ RSpec.describe 'Jobs', :clean_gitlab_redis_shared_state do
 
         it 'renders empty state' do
           expect(page).to have_content(job.detailed_status(user).illustration[:title])
-          expect(page).not_to have_selector('.js-build-trace')
+          expect(page).not_to have_selector('.job-log')
           expect(page).to have_content('This job has been canceled')
         end
       end
@@ -852,7 +852,7 @@ RSpec.describe 'Jobs', :clean_gitlab_redis_shared_state do
 
       it 'renders empty state' do
         expect(page).to have_content(job.detailed_status(user).illustration[:title])
-        expect(page).not_to have_selector('.js-build-trace')
+        expect(page).not_to have_selector('.job-log')
         expect(page).to have_content('This job has been skipped')
       end
     end

spec/frontend/jobs/components/job_app_spec.js

@@ -397,132 +397,31 @@ describe('Job App', () => {
     });
   });
 
-  describe('trace output', () => {
-    describe('with append flag', () => {
-      it('appends the log content to the existing one', () =>
-        setupAndMount({
-          traceData: {
-            html: '<span>More<span>',
-            status: 'running',
-            state: 'newstate',
-            append: true,
-            complete: true,
-          },
-        })
-          .then(() => {
-            store.state.trace = 'Update';
-
-            return wrapper.vm.$nextTick();
-          })
-          .then(() => {
-            expect(
-              wrapper
-                .find('.js-build-trace')
-                .text()
-                .trim(),
-            ).toEqual('Update');
-          }));
+  describe('trace controls', () => {
+    beforeEach(() =>
+      setupAndMount({
+        traceData: {
+          html: '<span>Update</span>',
+          status: 'success',
+          append: false,
+          size: 50,
+          total: 100,
+          complete: true,
+        },
+      }),
+    );
+
+    it('should render scroll buttons', () => {
+      expect(wrapper.find('.js-scroll-top').exists()).toBe(true);
+      expect(wrapper.find('.js-scroll-bottom').exists()).toBe(true);
     });
 
-    describe('without append flag', () => {
-      it('replaces the trace', () =>
-        setupAndMount({
-          traceData: {
-            html: '<span>Different<span>',
-            status: 'running',
-            append: false,
-            complete: true,
-          },
-        }).then(() => {
-          expect(
-            wrapper
-              .find('.js-build-trace')
-              .text()
-              .trim(),
-          ).toEqual('Different');
-        }));
-    });
-
-    describe('truncated information', () => {
-      describe('when size is less than total', () => {
-        it('shows information about truncated log', () => {
-          mock.onGet(`${props.pagePath}/trace.json`).reply(200, {
-            html: '<span>Update</span>',
-            status: 'success',
-            append: false,
-            size: 50,
-            total: 100,
-            complete: true,
-          });
-
-          return setupAndMount({
-            traceData: {
-              html: '<span>Update</span>',
-              status: 'success',
-              append: false,
-              size: 50,
-              total: 100,
-              complete: true,
-            },
-          }).then(() => {
-            expect(
-              wrapper
-                .find('.js-truncated-info')
-                .text()
-                .trim(),
-            ).toContain('Showing last 50 bytes');
-          });
-        });
-      });
-
-      describe('when size is equal than total', () => {
-        it('does not show the truncated information', () =>
-          setupAndMount({
-            traceData: {
-              html: '<span>Update</span>',
-              status: 'success',
-              append: false,
-              size: 100,
-              total: 100,
-              complete: true,
-            },
-          }).then(() => {
-            expect(
-              wrapper
-                .find('.js-truncated-info')
-                .text()
-                .trim(),
-            ).toEqual('');
-          }));
-      });
+    it('should render link to raw ouput', () => {
+      expect(wrapper.find('.js-raw-link-controller').exists()).toBe(true);
     });
 
-    describe('trace controls', () => {
-      beforeEach(() =>
-        setupAndMount({
-          traceData: {
-            html: '<span>Update</span>',
-            status: 'success',
-            append: false,
-            size: 50,
-            total: 100,
-            complete: true,
-          },
-        }),
-      );
-
-      it('should render scroll buttons', () => {
-        expect(wrapper.find('.js-scroll-top').exists()).toBe(true);
-        expect(wrapper.find('.js-scroll-bottom').exists()).toBe(true);
-      });
-
-      it('should render link to raw ouput', () => {
-        expect(wrapper.find('.js-raw-link-controller').exists()).toBe(true);
-      });
-
-      it('should render link to erase job', () => {
-        expect(wrapper.find('.js-erase-link').exists()).toBe(true);
-      });
+    it('should render link to erase job', () => {
+      expect(wrapper.find('.js-erase-link').exists()).toBe(true);
     });
   });
 });

spec/frontend/jobs/components/job_log_spec.js

-import Vue from 'vue';
-import { mountComponentWithStore } from 'helpers/vue_mount_component_helper';
-import component from '~/jobs/components/job_log.vue';
-import createStore from '~/jobs/store';
-import { resetStore } from '../store/helpers';
-
-describe('Job Log', () => {
-  const Component = Vue.extend(component);
-  let store;
-  let vm;
-
-  const trace =
-    '<span>Running with gitlab-runner 12.1.0 (de7731dd)<br/></span><span>  on docker-auto-scale-com d5ae8d25<br/></span><div class="gl-mr-3" data-timestamp="1565502765" data-section="prepare-executor" role="button"></div><span class="section section-header js-s-prepare-executor">Using Docker executor with image ruby:2.6 ...<br/></span>';
-
-  beforeEach(() => {
-    store = createStore();
-  });
-
-  afterEach(() => {
-    resetStore(store);
-    vm.$destroy();
-  });
-
-  it('renders provided trace', () => {
-    vm = mountComponentWithStore(Component, {
-      props: {
-        trace,
-        isComplete: true,
-      },
-      store,
-    });
-
-    expect(vm.$el.querySelector('code').textContent).toContain(
-      'Running with gitlab-runner 12.1.0 (de7731dd)',
-    );
-  });
-
-  describe('while receiving trace', () => {
-    it('renders animation', () => {
-      vm = mountComponentWithStore(Component, {
-        props: {
-          trace,
-          isComplete: false,
-        },
-        store,
-      });
-
-      expect(vm.$el.querySelector('.js-log-animation')).not.toBeNull();
-    });
-  });
-
-  describe('when build trace has finishes', () => {
-    it('does not render animation', () => {
-      vm = mountComponentWithStore(Component, {
-        props: {
-          trace,
-          isComplete: true,
-        },
-        store,
-      });
-
-      expect(vm.$el.querySelector('.js-log-animation')).toBeNull();
-    });
-  });
-});

spec/frontend/jobs/store/mutations_spec.js

@@ -76,28 +76,15 @@ describe('Jobs Store Mutations', () => {
         lines: [],
       });
 
-      expect(stateCopy.trace).toEqual(html);
       expect(stateCopy.traceSize).toEqual(511846);
       expect(stateCopy.isTraceComplete).toEqual(true);
     });
 
     describe('with new job log', () => {
-      let stateWithNewLog;
-      beforeEach(() => {
-        gon.features = gon.features || {};
-        gon.features.jobLogJson = true;
-
-        stateWithNewLog = state();
-      });
-
-      afterEach(() => {
-        gon.features.jobLogJson = false;
-      });
-
       describe('log.lines', () => {
         describe('when append is true', () => {
           it('sets the parsed log ', () => {
-            mutations[types.RECEIVE_TRACE_SUCCESS](stateWithNewLog, {
+            mutations[types.RECEIVE_TRACE_SUCCESS](stateCopy, {
               append: true,
               size: 511846,
               complete: true,
@@ -109,7 +96,7 @@ describe('Jobs Store Mutations', () => {
               ],
             });
 
-            expect(stateWithNewLog.trace).toEqual([
+            expect(stateCopy.trace).toEqual([
               {
                 offset: 1,
                 content: [{ text: 'Running with gitlab-runner 11.12.1 (5a147c92)' }],
@@ -121,7 +108,7 @@ describe('Jobs Store Mutations', () => {
 
         describe('when it is defined', () => {
           it('sets the parsed log ', () => {
-            mutations[types.RECEIVE_TRACE_SUCCESS](stateWithNewLog, {
+            mutations[types.RECEIVE_TRACE_SUCCESS](stateCopy, {
               append: false,
               size: 511846,
               complete: true,
@@ -130,7 +117,7 @@ describe('Jobs Store Mutations', () => {
               ],
             });
 
-            expect(stateWithNewLog.trace).toEqual([
+            expect(stateCopy.trace).toEqual([
               {
                 offset: 0,
                 content: [{ text: 'Running with gitlab-runner 11.11.1 (5a147c92)' }],
@@ -142,7 +129,7 @@ describe('Jobs Store Mutations', () => {
 
         describe('when it is null', () => {
           it('sets the default value', () => {
-            mutations[types.RECEIVE_TRACE_SUCCESS](stateWithNewLog, {
+            mutations[types.RECEIVE_TRACE_SUCCESS](stateCopy, {
               append: true,
               html,
               size: 511846,
@@ -150,7 +137,7 @@ describe('Jobs Store Mutations', () => {
               lines: null,
             });
 
-            expect(stateWithNewLog.trace).toEqual([]);
+            expect(stateCopy.trace).toEqual([]);
           });
         });
       });

spec/models/ci/build_trace_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Ci::BuildTrace do
     Gitlab::Ci::Trace::Stream.new { data }
   end
 
-  subject { described_class.new(build: build, stream: stream, state: state, content_format: content_format) }
+  subject { described_class.new(build: build, stream: stream, state: state) }
 
   shared_examples 'delegates methods' do
     it { is_expected.to delegate_method(:state).to(:trace) }
@@ -25,29 +25,11 @@ RSpec.describe Ci::BuildTrace do
     it { is_expected.to delegate_method(:complete?).to(:build).with_prefix }
   end
 
-  context 'with :json content format' do
-    let(:content_format) { :json }
+  it_behaves_like 'delegates methods'
 
-    it_behaves_like 'delegates methods'
-
-    it { is_expected.to be_json }
-
-    it 'returns formatted trace' do
-      expect(subject.trace.lines).to eq([
-        { offset: 0, content: [{ text: 'the-stream' }] }
-      ])
-    end
-  end
-
-  context 'with :html content format' do
-    let(:content_format) { :html }
-
-    it_behaves_like 'delegates methods'
-
-    it { is_expected.to be_html }
-
-    it 'returns formatted trace' do
-      expect(subject.trace.html).to eq('<span>the-stream</span>')
-    end
+  it 'returns formatted trace' do
+    expect(subject.lines).to eq([
+      { offset: 0, content: [{ text: 'the-stream' }] }
+    ])
   end
 end

spec/serializers/build_trace_entity_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe BuildTraceEntity do
   end
 
   let(:build_trace) do
-    Ci::BuildTrace.new(build: build, stream: stream, content_format: content_format, state: nil)
+    Ci::BuildTrace.new(build: build, stream: stream, state: nil)
   end
 
   let(:entity) do
@@ -22,42 +22,24 @@ RSpec.describe BuildTraceEntity do
 
   subject { entity.as_json }
 
-  shared_examples 'includes build and trace metadata' do
-    it 'includes build attributes' do
-      expect(subject[:id]).to eq(build.id)
-      expect(subject[:status]).to eq(build.status)
-      expect(subject[:complete]).to eq(build.complete?)
-    end
-
-    it 'includes trace metadata' do
-      expect(subject).to include(:state)
-      expect(subject).to include(:append)
-      expect(subject).to include(:truncated)
-      expect(subject).to include(:offset)
-      expect(subject).to include(:size)
-      expect(subject).to include(:total)
-    end
+  it 'includes build attributes' do
+    expect(subject[:id]).to eq(build.id)
+    expect(subject[:status]).to eq(build.status)
+    expect(subject[:complete]).to eq(build.complete?)
   end
 
-  context 'when content format is :json' do
-    let(:content_format) { :json }
-
-    it_behaves_like 'includes build and trace metadata'
-
-    it 'includes the trace content in json' do
-      expect(subject[:lines]).to eq([
-        { offset: 0, content: [{ text: 'the-trace' }] }
-      ])
-    end
+  it 'includes trace metadata' do
+    expect(subject).to include(:state)
+    expect(subject).to include(:append)
+    expect(subject).to include(:truncated)
+    expect(subject).to include(:offset)
+    expect(subject).to include(:size)
+    expect(subject).to include(:total)
   end
 
-  context 'when content format is :html' do
-    let(:content_format) { :html }
-
-    it_behaves_like 'includes build and trace metadata'
-
-    it 'includes the trace content in json' do
-      expect(subject[:html]).to eq('<span>the-trace</span>')
-    end
+  it 'includes the trace content in json' do
+    expect(subject[:lines]).to eq([
+      { offset: 0, content: [{ text: 'the-trace' }] }
+    ])
   end
 end