17 files changed, 206 insertions, 4 deletions

diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb
index 796f3ff603f..cb473d6ee96 100644
--- a/app/controllers/projects/jobs_controller.rb
+++ b/app/controllers/projects/jobs_controller.rb
@@ -51,6 +51,8 @@ class Projects::JobsController < Projects::ApplicationController
     build.trace.read do |stream|
       respond_to do |format|
         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
diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb
index 3e5a1cfc74d..72251988b5e 100644
--- a/app/controllers/projects/uploads_controller.rb
+++ b/app/controllers/projects/uploads_controller.rb
@@ -29,4 +29,14 @@ class Projects::UploadsController < Projects::ApplicationController
 
     Project.find_by_full_path("#{namespace}/#{id}")
   end
+
+  # Overrides ApplicationController#build_canonical_path since there are
+  # multiple routes that match project uploads:
+  # https://gitlab.com/gitlab-org/gitlab/issues/196396
+  def build_canonical_path(project)
+    return super unless action_name == 'show'
+    return super unless params[:secret] && params[:filename]
+
+    show_namespace_project_uploads_url(project.namespace.to_param, project.to_param, params[:secret], params[:filename])
+  end
 end
diff --git a/app/views/layouts/_flash.html.haml b/app/views/layouts/_flash.html.haml
index de1caeaa50f..07c271be2f0 100644
--- a/app/views/layouts/_flash.html.haml
+++ b/app/views/layouts/_flash.html.haml
@@ -1,10 +1,12 @@
 -# We currently only support `alert`, `notice`, `success`, 'toast'
+- icons = {'alert' => 'error', 'notice' => 'information-o', 'success' => 'check-circle'};
 .flash-container.flash-container-page.sticky
   - flash.each do |key, value|
     - if key == 'toast' && value
       .js-toast-message{ data: { message: value } }
     - elsif value
       %div{ class: "flash-#{key} mb-2" }
+        = sprite_icon(icons[key], size: 16, css_class: 'align-middle mr-1') unless icons[key].nil?
         %span= value
         %div{ class: "close-icon-wrapper js-close-icon" }
           = sprite_icon('close', size: 16, css_class: 'close-icon')
diff --git a/changelogs/unreleased/33658-reducing-build-trace-update-frequency.yml b/changelogs/unreleased/33658-reducing-build-trace-update-frequency.yml
new file mode 100644
index 00000000000..aff8ec5c918
--- /dev/null
+++ b/changelogs/unreleased/33658-reducing-build-trace-update-frequency.yml
@@ -0,0 +1,5 @@
+---
+title: Request less frequent updates from Runner when job log is not being watched
+merge_request: 20841
+author:
+type: performance
diff --git a/changelogs/unreleased/icons-to-alerts.yml b/changelogs/unreleased/icons-to-alerts.yml
new file mode 100644
index 00000000000..a7e8b713f6d
--- /dev/null
+++ b/changelogs/unreleased/icons-to-alerts.yml
@@ -0,0 +1,5 @@
+---
+title: Fix aligment for icons on alerts
+merge_request: 22760
+author: Rajendra Kadam
+type: added
diff --git a/changelogs/unreleased/sh-disable-formats-in-uploads-routes.yml b/changelogs/unreleased/sh-disable-formats-in-uploads-routes.yml
new file mode 100644
index 00000000000..4d0189749ac
--- /dev/null
+++ b/changelogs/unreleased/sh-disable-formats-in-uploads-routes.yml
@@ -0,0 +1,5 @@
+---
+title: Fix upload redirections when project has moved
+merge_request: 22822
+author:
+type: fixed
diff --git a/config/routes/group.rb b/config/routes/group.rb
index 30671d4e0a1..24957d5ecef 100644
--- a/config/routes/group.rb
+++ b/config/routes/group.rb
@@ -68,7 +68,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
 
     resources :uploads, only: [:create] do
       collection do
-        get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }
+        get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil }
         post :authorize
       end
     end
diff --git a/config/routes/project.rb b/config/routes/project.rb
index b420f07c7a1..3bc38ef3349 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -452,7 +452,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
 
       resources :uploads, only: [:create] do
         collection do
-          get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }
+          get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil }
           post :authorize
         end
       end
diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb
index 096ef146e07..fb8af76397c 100644
--- a/config/routes/uploads.rb
+++ b/config/routes/uploads.rb
@@ -21,9 +21,11 @@ scope path: :uploads do
       as: 'appearance_upload'
 
   # Project markdown uploads
+  # DEPRECATED: Remove this in GitLab 13.0 because this is redundant to show_namespace_project_uploads
+  # https://gitlab.com/gitlab-org/gitlab/issues/196396
   get ":namespace_id/:project_id/:secret/:filename",
-    to:           "projects/uploads#show",
-    constraints:  { namespace_id: /[a-zA-Z.0-9_\-]+/, project_id: /[a-zA-Z.0-9_\-]+/, filename: %r{[^/]+} }
+    to: redirect("%{namespace_id}/%{project_id}/uploads/%{secret}/%{filename}"),
+    constraints:  { namespace_id: /[a-zA-Z.0-9_\-]+/, project_id: /[a-zA-Z.0-9_\-]+/, filename: %r{[^/]+} }, format: false, defaults: { format: nil }
 
   # create uploads for models, snippets (notes) available for now
   post ':model',
diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index f383c541f8a..60cf9bf2c9c 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -200,6 +200,10 @@ module API
         status 202
         header 'Job-Status', job.status
         header 'Range', "0-#{stream_size}"
+
+        if Feature.enabled?(:runner_job_trace_update_interval_header, default_enabled: true)
+          header 'X-GitLab-Trace-Update-Interval', job.trace.update_interval.to_s
+        end
       end
 
       desc 'Authorize artifacts uploading for job' do
diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb
index 941f7178dac..4e83826b249 100644
--- a/lib/gitlab/ci/trace.rb
+++ b/lib/gitlab/ci/trace.rb
@@ -9,6 +9,10 @@ module Gitlab
       LOCK_TTL = 10.minutes
       LOCK_RETRIES = 2
       LOCK_SLEEP = 0.001.seconds
+      WATCH_FLAG_TTL = 10.seconds
+
+      UPDATE_FREQUENCY_DEFAULT = 30.seconds
+      UPDATE_FREQUENCY_WHEN_BEING_WATCHED = 3.seconds
 
       ArchiveError = Class.new(StandardError)
       AlreadyArchivedError = Class.new(StandardError)
@@ -119,6 +123,22 @@ module Gitlab
         end
       end
 
+      def update_interval
+        being_watched? ? UPDATE_FREQUENCY_WHEN_BEING_WATCHED : UPDATE_FREQUENCY_DEFAULT
+      end
+
+      def being_watched!
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.set(being_watched_cache_key, true, ex: WATCH_FLAG_TTL)
+        end
+      end
+
+      def being_watched?
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.exists(being_watched_cache_key)
+        end
+      end
+
       private
 
       def unsafe_write!(mode, &blk)
@@ -236,6 +256,10 @@ module Gitlab
       def trace_artifact
         job.job_artifacts_trace
       end
+
+      def being_watched_cache_key
+        "gitlab:ci:trace:#{job.id}:watched"
+      end
     end
   end
 end
diff --git a/spec/controllers/groups/uploads_controller_spec.rb b/spec/controllers/groups/uploads_controller_spec.rb
index 60342bf8e3d..8abebd04e8b 100644
--- a/spec/controllers/groups/uploads_controller_spec.rb
+++ b/spec/controllers/groups/uploads_controller_spec.rb
@@ -19,6 +19,22 @@ describe Groups::UploadsController do
     let(:uploader_class) { NamespaceFileUploader }
   end
 
+  context 'with a moved group' do
+    let!(:upload) { create(:upload, :issuable_upload, :with_file, model: model) }
+    let(:group) { model }
+    let(:old_path) { group.to_param + 'old' }
+    let!(:redirect_route) { model.redirect_routes.create(path: old_path) }
+    let(:upload_path) { File.basename(upload.path) }
+
+    it 'redirects to a file with the proper extension' do
+      get :show, params: { group_id: old_path, filename: upload_path, secret: upload.secret }
+
+      expect(response.location).to eq(show_group_uploads_url(group, upload.secret, upload_path))
+      expect(response.location).to end_with(upload.path)
+      expect(response).to have_gitlab_http_status(:redirect)
+    end
+  end
+
   def post_authorize(verified: true)
     request.headers.merge!(workhorse_internal_api_request_header) if verified
 
diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb
index edef24f6595..53c40683a5b 100644
--- a/spec/controllers/projects/jobs_controller_spec.rb
+++ b/spec/controllers/projects/jobs_controller_spec.rb
@@ -556,6 +556,12 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
         expect(json_response['status']).to eq job.status
         expect(json_response['lines']).to eq [{ 'content' => [{ 'text' => 'BUILD TRACE' }], 'offset' => 0 }]
       end
+
+      it 'sets being-watched flag for the job' do
+        expect(response).to have_gitlab_http_status(:ok)
+
+        expect(job.trace.being_watched?).to be(true)
+      end
     end
 
     context 'when job has no traces' do
diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb
index cd6a9886f72..a70669e86a6 100644
--- a/spec/controllers/projects/uploads_controller_spec.rb
+++ b/spec/controllers/projects/uploads_controller_spec.rb
@@ -25,6 +25,21 @@ describe Projects::UploadsController do
     end
   end
 
+  context 'with a moved project' do
+    let!(:upload) { create(:upload, :issuable_upload, :with_file, model: model) }
+    let(:project) { model }
+    let(:upload_path) { File.basename(upload.path) }
+    let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') }
+
+    it 'redirects to a file with the proper extension' do
+      get :show, params: { namespace_id: project.namespace, project_id: project.to_param + 'old', filename: File.basename(upload.path), secret: upload.secret }
+
+      expect(response.location).to eq(show_project_uploads_url(project, upload.secret, upload_path))
+      expect(response.location).to end_with(upload.path)
+      expect(response).to have_gitlab_http_status(:redirect)
+    end
+  end
+
   context "when exception occurs" do
     before do
       allow(FileUploader).to receive(:workhorse_authorize).and_raise(SocketError.new)
diff --git a/spec/lib/gitlab/ci/trace_spec.rb b/spec/lib/gitlab/ci/trace_spec.rb
index f7bc5686b68..574c2b73722 100644
--- a/spec/lib/gitlab/ci/trace_spec.rb
+++ b/spec/lib/gitlab/ci/trace_spec.rb
@@ -26,4 +26,66 @@ describe Gitlab::Ci::Trace, :clean_gitlab_redis_shared_state do
 
     it_behaves_like 'trace with enabled live trace feature'
   end
+
+  describe '#update_interval' do
+    context 'it is not being watched' do
+      it 'returns 30 seconds' do
+        expect(trace.update_interval).to eq(30.seconds)
+      end
+    end
+
+    context 'it is being watched' do
+      before do
+        trace.being_watched!
+      end
+
+      it 'returns 3 seconds' do
+        expect(trace.update_interval).to eq(3.seconds)
+      end
+    end
+  end
+
+  describe '#being_watched!' do
+    let(:cache_key) { "gitlab:ci:trace:#{build.id}:watched" }
+
+    it 'sets gitlab:ci:trace:<job.id>:watched in redis' do
+      trace.being_watched!
+
+      result = Gitlab::Redis::SharedState.with do |redis|
+        redis.exists(cache_key)
+      end
+
+      expect(result).to eq(true)
+    end
+
+    it 'updates the expiry of gitlab:ci:trace:<job.id>:watched in redis', :clean_gitlab_redis_shared_state do
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.set(cache_key, true, ex: 4.seconds)
+      end
+
+      expect do
+        trace.being_watched!
+      end.to change { Gitlab::Redis::SharedState.with { |redis| redis.pttl(cache_key) } }
+    end
+  end
+
+  describe '#being_watched?' do
+    context 'gitlab:ci:trace:<job.id>:watched in redis is set', :clean_gitlab_redis_shared_state do
+      before do
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.set("gitlab:ci:trace:#{build.id}:watched", true)
+        end
+      end
+
+      it 'returns true' do
+        expect(trace.being_watched?).to be(true)
+      end
+    end
+
+    context 'gitlab:ci:trace:<job.id>:watched in redis is not set' do
+      it 'returns false' do
+        expect(trace.being_watched?).to be(false)
+      end
+    end
+  end
 end
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index cc6cadb190a..93b35401706 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -1154,6 +1154,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
           expect(job.reload.trace.raw).to eq 'BUILD TRACE appended'
           expect(response.header).to have_key 'Range'
           expect(response.header).to have_key 'Job-Status'
+          expect(response.header).to have_key 'X-GitLab-Trace-Update-Interval'
         end
 
         context 'when job has been updated recently' do
@@ -1291,6 +1292,41 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             expect(response.header['Job-Status']).to eq 'canceled'
           end
         end
+
+        context 'when build trace is being watched' do
+          before do
+            job.trace.being_watched!
+          end
+
+          it 'returns X-GitLab-Trace-Update-Interval as 3' do
+            patch_the_trace
+
+            expect(response.status).to eq 202
+            expect(response.header['X-GitLab-Trace-Update-Interval']).to eq('3')
+          end
+        end
+
+        context 'when build trace is not being watched' do
+          it 'returns X-GitLab-Trace-Update-Interval as 30' do
+            patch_the_trace
+
+            expect(response.status).to eq 202
+            expect(response.header['X-GitLab-Trace-Update-Interval']).to eq('30')
+          end
+        end
+
+        context 'when feature flag runner_job_trace_update_interval_header is disabled' do
+          before do
+            stub_feature_flags(runner_job_trace_update_interval_header: { enabled: false })
+          end
+
+          it 'does not return X-GitLab-Trace-Update-Interval header' do
+            patch_the_trace
+
+            expect(response.status).to eq 202
+            expect(response.header).not_to have_key 'X-GitLab-Trace-Update-Interval'
+          end
+        end
       end
 
       context 'when Runner makes a force-patch' do
diff --git a/spec/routing/uploads_routing_spec.rb b/spec/routing/uploads_routing_spec.rb
index 42e84774088..f94ae81eeb5 100644
--- a/spec/routing/uploads_routing_spec.rb
+++ b/spec/routing/uploads_routing_spec.rb
@@ -28,4 +28,12 @@ describe 'Uploads', 'routing' do
       expect(post("/uploads/#{model}?id=1")).not_to be_routable
     end
   end
+
+  describe 'legacy paths' do
+    include RSpec::Rails::RequestExampleGroup
+
+    it 'redirects project uploads to canonical path under project namespace' do
+      expect(get('/uploads/namespace/project/12345/test.png')).to redirect_to('/namespace/project/uploads/12345/test.png')
+    end
+  end
 end