Merge branch '4782-fix-geo-node-status-version-and-revision' into 'master'

Report the correct version and revision for Geo node status requests

Closes #4782

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

changelogs/unreleased-ee/4782-fix-geo-node-status-version-and-revision.yml

+---
+title: Report the correct version and revision for Geo node status requests
+merge_request: 4353
+author:
+type: fixed

db/schema.rb

@@ -999,6 +999,8 @@ ActiveRecord::Schema.define(version: 20180202111106) do
     t.integer "job_artifacts_count"
     t.integer "job_artifacts_synced_count"
     t.integer "job_artifacts_failed_count"
+    t.string "version"
+    t.string "revision"
   end
 
   add_index "geo_node_statuses", ["geo_node_id"], name: "index_geo_node_statuses_on_geo_node_id", unique: true, using: :btree

ee/app/helpers/ee/geo_helper.rb

 module EE
   module GeoHelper
+    def node_vue_list_properties
+      version, revision =
+        if ::Gitlab::Geo.primary?
+          [::Gitlab::VERSION, ::Gitlab::REVISION]
+        else
+          status = ::Gitlab::Geo.primary_node&.status
+
+          [status&.version, status&.revision]
+        end
+
+      {
+        primary_version: version.to_s,
+        primary_revision: revision.to_s,
+        node_details_path: admin_geo_nodes_path.to_s,
+        node_actions_allowed: ::Gitlab::Database.read_write?.to_s,
+        node_edit_allowed: ::Gitlab::Geo.license_allows?.to_s
+      }
+    end
+
     def node_namespaces_options(namespaces)
       namespaces.map { |g| { id: g.id, text: g.full_name } }
     end

ee/app/models/geo_node_status.rb

@@ -4,7 +4,7 @@ class GeoNodeStatus < ActiveRecord::Base
   delegate :selective_sync_type, to: :geo_node
 
   # Whether we were successful in reaching this node
-  attr_accessor :success, :version, :revision
+  attr_accessor :success
   attr_writer :health_status
   attr_accessor :storage_shards
 
@@ -58,10 +58,18 @@ class GeoNodeStatus < ActiveRecord::Base
     GeoNodeStatus.new(HashWithIndifferentAccess.new(json_data))
   end
 
+  EXCLUDED_PARAMS = %w[id created_at].freeze
+  EXTRA_PARAMS = %w[
+    success
+    health
+    health_status
+    last_event_timestamp
+    cursor_last_event_timestamp
+    storage_shards
+  ].freeze
+
   def self.allowed_params
-    excluded_params = %w(id created_at updated_at).freeze
-    extra_params = %w(success health health_status last_event_timestamp cursor_last_event_timestamp version revision storage_shards updated_at).freeze
-    self.column_names - excluded_params + extra_params
+    self.column_names - EXCLUDED_PARAMS + EXTRA_PARAMS
   end
 
   def load_data_from_current_node
@@ -83,6 +91,9 @@ class GeoNodeStatus < ActiveRecord::Base
     self.last_successful_status_check_at = Time.now
     self.storage_shards = StorageShard.all
 
+    self.version = Gitlab::VERSION
+    self.revision = Gitlab::REVISION
+
     load_primary_data
     load_secondary_data
 

ee/app/views/admin/geo_nodes/index.html.haml

@@ -23,6 +23,6 @@
     .alert.alert-warning WARNING: Please upgrade PostgreSQL to version 9.6 or greater. The status of the replication cannot be determined reliably with the current version.
 
 - if @nodes.any?
-  #js-geo-nodes{ data: { primary_version: "#{Gitlab::VERSION}", primary_revision: "#{Gitlab::REVISION}", node_details_path: "#{admin_geo_nodes_path}", node_actions_allowed: "#{Gitlab::Database.read_write?}", node_edit_allowed: "#{Gitlab::Geo.license_allows?}" } }
+  #js-geo-nodes{ data: node_vue_list_properties }
 - else
   = render 'shared/empty_states/geo'

ee/db/migrate/20180201192230_store_version_and_revision_in_geo_node_status.rb

+class StoreVersionAndRevisionInGeoNodeStatus < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def change
+    add_column :geo_node_statuses, :version, :string
+    add_column :geo_node_statuses, :revision, :string
+  end
+end

lib/api/entities.rb

@@ -1267,14 +1267,6 @@ module API
       def missing_oauth_application
         object.geo_node.missing_oauth_application?
       end
-
-      def version
-        Gitlab::VERSION
-      end
-
-      def revision
-        Gitlab::REVISION
-      end
     end
 
     class PersonalAccessToken < Grape::Entity

spec/ee/spec/models/geo_node_status_spec.rb

@@ -12,7 +12,7 @@ describe GeoNodeStatus, :geo do
   let!(:project_3) { create(:project) }
   let!(:project_4) { create(:project) }
 
-  subject { described_class.current_node_status }
+  subject(:status) { described_class.current_node_status }
 
   before do
     stub_current_geo_node(secondary)
@@ -425,6 +425,14 @@ describe GeoNodeStatus, :geo do
     end
   end
 
+  describe '#version' do
+    it { expect(status.version).to eq(Gitlab::VERSION) }
+  end
+
+  describe '#revision' do
+    it {  expect(status.revision).to eq(Gitlab::REVISION) }
+  end
+
   describe '#[]' do
     it 'returns values for each attribute' do
       expect(subject[:repositories_count]).to eq(4)

spec/factories/geo_node_statuses.rb

@@ -25,6 +25,8 @@ FactoryBot.define do
       cursor_last_event_id 1
       cursor_last_event_timestamp { Time.now.to_i }
       last_successful_status_check_timestamp { Time.now.beginning_of_day }
+      version { Gitlab::VERSION }
+      revision { Gitlab::REVISION }
     end
 
     trait :unhealthy do

spec/requests/api/geo_nodes_spec.rb

@@ -69,6 +69,7 @@ describe API::GeoNodes, :geo, api: true do
   describe 'GET /geo_nodes/:id/status' do
     it 'retrieves the Geo nodes status if admin is logged in' do
       stub_current_geo_node(primary)
+      secondary_status.update!(version: 'secondary-version', revision: 'secondary-revision')
 
       expect(GeoNodeStatus).not_to receive(:current_node_status)
 
@@ -77,7 +78,11 @@ describe API::GeoNodes, :geo, api: true do
       expect(response).to have_gitlab_http_status(200)
       expect(response).to match_response_schema('public_api/v4/geo_node_status', dir: 'ee')
 
+      expect(json_response['version']).to eq('secondary-version')
+      expect(json_response['revision']).to eq('secondary-revision')
+
       links = json_response['_links']
+
       expect(links['self']).to end_with("/api/v4/geo_nodes/#{secondary.id}/status")
       expect(links['node']).to end_with("/api/v4/geo_nodes/#{secondary.id}")
     end