Merge branch 'ce-to-ee-2018-05-23' into 'master'

CE upstream - 2018-05-23 15:36 UTC

See merge request gitlab-org/gitlab-ee!5824

app/controllers/projects/prometheus/metrics_controller.rb

@@ -27,7 +27,7 @@ module Projects
       end
 
       def require_prometheus_metrics!
-        render_404 unless prometheus_adapter.can_query?
+        render_404 unless prometheus_adapter&.can_query?
       end
     end
   end

changelogs/unreleased/46413-error-when-prometheus-adapter-is-disabled.yml

+---
+title: Render 404 when prometheus adapter is disabled in Prometheus metrics controller
+merge_request: 19110
+author:
+type: fixed

doc/development/ee_features.md

@@ -53,6 +53,9 @@ stub_licensed_features(variable_environment_scope: true)
 
 EE-specific comments should not be backported to CE.
 
+**Note:** This is only meant as a workaround, we should follow up and
+resolve this soon.
+
 ### Detection of EE-only files
 
 For each commit (except on `master`), the `ee-files-location-check` CI job tries
@@ -105,11 +108,14 @@ is applied not only to models. Here's a list of other examples:
 - `ee/app/services/foo/create_service.rb`
 - `ee/app/validators/foo_attr_validator.rb`
 - `ee/app/workers/foo_worker.rb`
+- `ee/app/views/foo.html.haml`
+- `ee/app/views/foo/_bar.html.haml`
 
 This works because for every path that are present in CE's eager-load/auto-load
 paths, we add the same `ee/`-prepended path in [`config/application.rb`].
+This also applies to views.
 
-[`config/application.rb`]: https://gitlab.com/gitlab-org/gitlab-ee/blob/d278b76d6600a0e27d8019a0be27971ba23ab640/config/application.rb#L41-51
+[`config/application.rb`]: https://gitlab.com/gitlab-org/gitlab-ee/blob/925d3d4ebc7a2c72964ce97623ae41b8af12538d/config/application.rb#L42-52
 
 ### EE features based on CE features
 
@@ -359,9 +365,37 @@ Blocks of code that are EE-specific should be moved to partials. This
 avoids conflicts with big chunks of HAML code that that are not fun to
 resolve when you add the indentation to the equation.
 
-EE-specific views should be placed in `ee/app/views/ee/`, using extra
+EE-specific views should be placed in `ee/app/views/`, using extra
 sub-directories if appropriate.
 
+Instead of using regular `render`, we should use `render_if_exists`, which
+will not render anything if it cannot find the specific partial. We use this
+so that we could put `render_if_exists` in CE, keeping code the same between
+CE and EE.
+
+Also, it should search for the EE partial first, and then CE partial, and
+then if nothing found, render nothing.
+
+This has two uses:
+
+- CE renders nothing, and EE renders its EE partial.
+- CE renders its CE partial, and EE renders its EE partial, while the view
+  file stays the same.
+
+The advantages of this:
+
+- Minimal code difference between CE and EE.
+- Very clear hints about where we're extending EE views while reading CE codes.
+- Whenever we want to show something different in CE, we could just add CE
+  partials. Same applies the other way around. If we just use
+  `render_if_exists`, it would be very easy to change the content in EE.
+
+The disadvantage of this:
+
+- Slightly more work while developing EE features, because now we need to
+  port `render_if_exists` to CE.
+- If we have typos in the partial name, it would be silently ignored.
+
 ### Code in `lib/`
 
 Place EE-specific logic in the top-level `EE` module namespace. Namespace the

spec/controllers/projects/prometheus/metrics_controller_spec.rb

@@ -14,44 +14,67 @@ describe Projects::Prometheus::MetricsController do
   end
 
   describe 'GET #active_common' do
-    before do
-      allow(controller).to receive(:prometheus_adapter).and_return(prometheus_adapter)
-    end
+    context 'when prometheus_adapter can query' do
+      before do
+        allow(controller).to receive(:prometheus_adapter).and_return(prometheus_adapter)
+      end
 
-    context 'when prometheus metrics are enabled' do
-      context 'when data is not present' do
-        before do
-          allow(prometheus_adapter).to receive(:query).with(:matched_metrics).and_return({})
-        end
+      context 'when prometheus metrics are enabled' do
+        context 'when data is not present' do
+          before do
+            allow(prometheus_adapter).to receive(:query).with(:matched_metrics).and_return({})
+          end
 
-        it 'returns no content response' do
-          get :active_common, project_params(format: :json)
+          it 'returns no content response' do
+            get :active_common, project_params(format: :json)
 
-          expect(response).to have_gitlab_http_status(204)
+            expect(response).to have_gitlab_http_status(204)
+          end
         end
-      end
 
-      context 'when data is available' do
-        let(:sample_response) { { some_data: 1 } }
+        context 'when data is available' do
+          let(:sample_response) { { some_data: 1 } }
+
+          before do
+            allow(prometheus_adapter).to receive(:query).with(:matched_metrics).and_return(sample_response)
+          end
 
-        before do
-          allow(prometheus_adapter).to receive(:query).with(:matched_metrics).and_return(sample_response)
+          it 'returns no content response' do
+            get :active_common, project_params(format: :json)
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response).to eq(sample_response.deep_stringify_keys)
+          end
         end
 
-        it 'returns no content response' do
-          get :active_common, project_params(format: :json)
+        context 'when requesting non json response' do
+          it 'returns not found response' do
+            get :active_common, project_params
 
-          expect(response).to have_gitlab_http_status(200)
-          expect(json_response).to eq(sample_response.deep_stringify_keys)
+            expect(response).to have_gitlab_http_status(404)
+          end
         end
       end
+    end
 
-      context 'when requesting non json response' do
-        it 'returns not found response' do
-          get :active_common, project_params
+    context 'when prometheus_adapter cannot query' do
+      it 'renders 404' do
+        prometheus_adapter = double('prometheus_adapter', can_query?: false)
 
-          expect(response).to have_gitlab_http_status(404)
-        end
+        allow(controller).to receive(:prometheus_adapter).and_return(prometheus_adapter)
+        allow(prometheus_adapter).to receive(:query).with(:matched_metrics).and_return({})
+
+        get :active_common, project_params(format: :json)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
+    context 'when prometheus_adapter is disabled' do
+      it 'renders 404' do
+        get :active_common, project_params(format: :json)
+
+        expect(response).to have_gitlab_http_status(404)
       end
     end
   end