gpconfig: Update message when file has no GUC value

Because users may find it confusing when gpconfig prints '-' or 'None'
when no GUC value is found in the file, this commit updates gpconfig to
output a clearer message.

Apply the out-of-band messaging to failure reports as well, and fix up
the output of --file-compare. Add tests for each modified
implementation.
Co-Authored-By: NJamie McAtamney <jmcatamney@pivotal.io>
Co-Authored-By: NMark Sliva <msliva@pivotal.io>
Co-Authored-By: NShoaib Lari <slari@pivotal.io>
(cherry picked from commit b8bd3577)

gpMgmt/bin/gpconfig_modules/compare_segment_guc.py

@@ -70,10 +70,16 @@ class MultiValueGuc(SegmentGuc):
 
     def report_success_format(self):
         file_val = self.primary_file_seg_guc.get_value()
-        if self.db_seg_guc:
-            result = "%s value: %s | file: %s" % (self.get_label(), self.db_seg_guc.value, self._use_dash_when_none(file_val))
+        if file_val is not None:
+            if self.db_seg_guc:
+                result = "%s value: %s | file: %s" % (self.get_label(), self.db_seg_guc.value, file_val)
+            else:
+                result = "%s value: %s" % (self.get_label(), file_val)
         else:
-            result = "%s value: %s" % (self.get_label(), file_val)
+            if self.db_seg_guc:
+                result = "%s value: %s | not set in file" % (self.get_label(), self.db_seg_guc.value)
+            else:
+                result = "No value is set on %s" % ("master" if self.get_label() == "Master " else "segments")
         return result
 
     def report_fail_format(self):
@@ -87,16 +93,17 @@ class MultiValueGuc(SegmentGuc):
         return report
 
     def _report_fail_format_with_database_and_file_gucs(self, segment_guc_obj):
-        return "[context: %s] [dbid: %s] [name: %s] [value: %s | file: %s]" % (
+        if segment_guc_obj.value is None:
+            file_tag = "not set in file"
+        else:
+            file_tag = "file: %s" % segment_guc_obj.value
+
+        return "[context: %s] [dbid: %s] [name: %s] [value: %s | %s]" % (
             self.db_seg_guc.context,
             segment_guc_obj.dbid,
             self.db_seg_guc.name,
             self.db_seg_guc.value,
-            self._use_dash_when_none(segment_guc_obj.value))
-
-    def _use_dash_when_none(self, value):
-      return value if value is not None else "-"
-
+            file_tag)
 
     def is_internally_consistent(self):
         if not self.db_seg_guc:

gpMgmt/bin/gpconfig_modules/database_segment_guc.py

@@ -12,7 +12,9 @@ class DatabaseSegmentGuc(SegmentGuc):
         self.value = row[2]
 
     def report_success_format(self):
-        return "%s value: %s" % (self.get_label(), self.get_value())
+        if self.get_value() is not None:
+            return "%s value: %s" % (self.get_label(), self.get_value())
+        return "No value is set on %s" % ("master" if self.get_label() == "Master " else "segments")
 
     def report_fail_format(self):
         return ["[context: %s] [name: %s] [value: %s]" % (self.context, self.name, self.get_value())]

gpMgmt/bin/gpconfig_modules/file_segment_guc.py

@@ -13,17 +13,21 @@ class FileSegmentGuc(SegmentGuc):
         self.dbid = str(row[3])
 
     def report_success_format(self):
-        return "%s value: %s" % (self.get_label(), self._use_dash_when_none(self.get_value()))
+        if self.get_value() is not None:
+            return "%s value: %s" % (self.get_label(), self.get_value())
+        return "No value is set on %s" % ("master" if self.get_label() == "Master " else "segments")
 
     def report_fail_format(self):
-        return ["[context: %s] [dbid: %s] [name: %s] [value: %s]" % (self.context, self.dbid, self.name, self._use_dash_when_none(self.get_value()))]
+        value = self.get_value()
+        if value is not None:
+            value = 'value: %s' % value
+        else:
+            value = 'not set in file'
+
+        return ["[context: %s] [dbid: %s] [name: %s] [%s]" % (self.context, self.dbid, self.name, value)]
 
     def is_internally_consistent(self):
         return True
 
     def get_value(self):
         return self.value
-
-    def _use_dash_when_none(self, value):
-        return value if value is not None else "-"
-

gpMgmt/bin/gppylib/test/unit/test_unit_file_segment_guc.py

@@ -21,6 +21,11 @@ class FileSegmentGucTest(GpTestCase):
         self.assertEquals(self.subject.report_fail_format(),
                           ["[context: contentid] [dbid: dbid] [name: guc_name] [value: value_from_file]"])
 
+    def test_report_fail_format_file_when_unset(self):
+        self.subject.value = None
+        self.assertEquals(self.subject.report_fail_format(),
+                          ["[context: contentid] [dbid: dbid] [name: guc_name] [not set in file]"])
+
     def test_init_with_insufficient_file_values_raises(self):
         row = ['contentid', 'guc_name', 'value_from_file']
         with self.assertRaisesRegexp(Exception, "must provide \['context', 'guc name', 'value', 'dbid'\]"):
@@ -33,7 +38,7 @@ class FileSegmentGucTest(GpTestCase):
         self.assertEquals(self.subject.report_success_format(), "Master  value: value")
         self.assertEqual(self.subject.dbid, '0')
 
-    def test_when_value_none_report_success_uses_hyphen(self):
+    def test_when_value_none_report_success_prints_message(self):
         self.subject.value = None
 
-        self.assertEquals(self.subject.report_success_format(), "Segment value: -")
+        self.assertEquals(self.subject.report_success_format(), "No value is set on segments")

gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py

@@ -181,15 +181,37 @@ class GpConfig(GpTestCase):
         self.assertIn("Master  value: foo\nSegment value: foo", mock_stdout.getvalue())
 
     @patch('sys.stdout', new_callable=StringIO)
-    def test_option_f_will_report_absence_of_setting(self, mock_stdout):
+    def test_option_f_will_report_absence_of_setting_on_master(self, mock_stdout):
         sys.argv = ["gpconfig", "--show", "my_property_name", "--file"]
-        self.master_read_config.get_guc_value.return_value = "-"
+        self.master_read_config.get_guc_value.return_value = None
         self.segment_read_config.get_guc_value.return_value = "seg_value"
 
         self.subject.do_main()
 
         self.assertEqual(self.subject.LOGGER.error.call_count, 0)
-        self.assertIn("Master  value: -\nSegment value: seg_value", mock_stdout.getvalue())
+        self.assertIn("No value is set on master\nSegment value: seg_value", mock_stdout.getvalue())
+
+    @patch('sys.stdout', new_callable=StringIO)
+    def test_option_f_will_report_absence_of_setting_on_segment(self, mock_stdout):
+        sys.argv = ["gpconfig", "--show", "my_property_name", "--file"]
+        self.master_read_config.get_guc_value.return_value = "master_value"
+        self.segment_read_config.get_guc_value.return_value = None
+
+        self.subject.do_main()
+
+        self.assertEqual(self.subject.LOGGER.error.call_count, 0)
+        self.assertIn("Master  value: master_value\nNo value is set on segments", mock_stdout.getvalue())
+
+    @patch('sys.stdout', new_callable=StringIO)
+    def test_option_f_will_report_absence_of_setting_on_both(self, mock_stdout):
+        sys.argv = ["gpconfig", "--show", "my_property_name", "--file"]
+        self.master_read_config.get_guc_value.return_value = None
+        self.segment_read_config.get_guc_value.return_value = None
+
+        self.subject.do_main()
+
+        self.assertEqual(self.subject.LOGGER.error.call_count, 0)
+        self.assertIn("No value is set on master\nNo value is set on segments", mock_stdout.getvalue())
 
     @patch('sys.stdout', new_callable=StringIO)
     def test_option_f_will_report_difference_segments_out_of_sync(self, mock_stdout):
@@ -210,6 +232,25 @@ class GpConfig(GpTestCase):
         self.assertIn("bar", mock_stdout.getvalue())
         self.assertIn("[name: my_property_name] [value: baz]", mock_stdout.getvalue())
 
+    @patch('sys.stdout', new_callable=StringIO)
+    def test_option_f_will_report_difference_segments_out_of_sync_when_unset(self, mock_stdout):
+        sys.argv = ["gpconfig", "--show", "my_property_name", "--file"]
+        self.master_read_config.get_guc_value.return_value = 'foo'
+        self.segment_read_config.get_guc_value.return_value = 'bar'
+        another_segment_read_config = Mock()
+        another_segment_read_config.get_guc_value.return_value = None
+        another_segment_read_config.get_seg_content_id.return_value = 1
+        self.pool.getCompletedItems.return_value.append(another_segment_read_config)
+        self.host_cache.get_hosts.return_value.extend([self.host, self.host])
+
+        self.subject.do_main()
+
+        self.assertEqual(self.pool.addCommand.call_count, 3)
+        self.assertEqual(self.subject.LOGGER.error.call_count, 0)
+        self.assertIn("WARNING: GUCS ARE OUT OF SYNC", mock_stdout.getvalue())
+        self.assertIn("bar", mock_stdout.getvalue())
+        self.assertIn("[name: my_property_name] [not set in file]", mock_stdout.getvalue())
+
     def test_option_change_value_master_separate_succeed(self):
         db_singleton_side_effect_list.append("some happy result")
         entry = 'my_property_name'
@@ -306,6 +347,30 @@ class GpConfig(GpTestCase):
         self.assertIn("Segment value: foo | file: foo", mock_stdout.getvalue())
         self.assertIn("Values on all segments are consistent", mock_stdout.getvalue())
 
+    @patch('sys.stdout', new_callable=StringIO)
+    def test_option_file_compare_works_with_unset_values(self, mock_stdout):
+        sys.argv = ["gpconfig", "-s", "my_property_name", "--file-compare"]
+        self.master_read_config.get_guc_value.return_value = None
+        self.master_read_config.get_seg_content_id.return_value = -1
+
+        self.segment_read_config.get_guc_value.return_value = None
+        self.segment_read_config.get_seg_content_id.return_value = 0
+
+        another_segment_read_config = Mock()
+        another_segment_read_config.get_guc_value.return_value = None
+        another_segment_read_config.get_seg_content_id.return_value = 1
+        self.pool.getCompletedItems.return_value.append(another_segment_read_config)
+
+        self.cursor.set_result_for_testing([[-1, 'my_property_name', 'foo'],
+                                            [0, 'my_property_name', 'foo'],
+                                            [1, 'my_property_name', 'foo']])
+
+        self.subject.do_main()
+
+        self.assertIn("Master  value: foo | not set in file", mock_stdout.getvalue())
+        self.assertIn("Segment value: foo | not set in file", mock_stdout.getvalue())
+        self.assertIn("Values on all segments are consistent", mock_stdout.getvalue())
+
     @patch('sys.stdout', new_callable=StringIO)
     def test_option_file_compare_returns_different_value(self, mock_stdout):
         sys.argv = ["gpconfig", "-s", "my_property_name", "--file-compare"]
@@ -337,6 +402,37 @@ class GpConfig(GpTestCase):
         self.assertIn("[context: 1] [dbid: 2] [name: my_property_name] [value: foo | file: bar]",
                       mock_stdout.getvalue())
 
+    @patch('sys.stdout', new_callable=StringIO)
+    def test_option_file_compare_with_unset_values_on_some_segments(self, mock_stdout):
+        sys.argv = ["gpconfig", "-s", "my_property_name", "--file-compare"]
+        self.master_read_config.get_guc_value.return_value = 'foo'
+        self.master_read_config.get_seg_content_id.return_value = -1
+        self.master_read_config.get_seg_dbid.return_value = 0
+
+        self.segment_read_config.get_guc_value.return_value = 'foo'
+        self.segment_read_config.get_seg_content_id.return_value = 0
+        self.segment_read_config.get_seg_dbid.return_value = 1
+
+        another_segment_read_config = Mock()
+        another_segment_read_config.get_guc_value.return_value = None
+        another_segment_read_config.get_seg_content_id.return_value = 1
+        another_segment_read_config.get_seg_dbid.return_value = 2
+        self.pool.getCompletedItems.return_value.append(another_segment_read_config)
+
+        self.cursor.set_result_for_testing([[-1, 'my_property_name', 'foo'],
+                                            [0, 'my_property_name', 'foo'],
+                                            [1, 'my_property_name', 'foo']])
+
+        self.subject.do_main()
+
+        self.assertIn("WARNING: GUCS ARE OUT OF SYNC: ", mock_stdout.getvalue())
+        self.assertIn("[context: -1] [dbid: 0] [name: my_property_name] [value: foo | file: foo]",
+                      mock_stdout.getvalue())
+        self.assertIn("[context: 0] [dbid: 1] [name: my_property_name] [value: foo | file: foo]",
+                      mock_stdout.getvalue())
+        self.assertIn("[context: 1] [dbid: 2] [name: my_property_name] [value: foo | not set in file]",
+                      mock_stdout.getvalue())
+
     @patch('sys.stdout', new_callable=StringIO)
     def test_option_file_compare_with_standby_master_with_different_file_value_will_report_failure(self, mock_stdout):
         sys.argv = ["gpconfig", "-s", "my_property_name", "--file-compare"]

gpMgmt/bin/gppylib/test/unit/test_unit_guccollection.py

@@ -89,8 +89,7 @@ class GucCollectionTest(GpTestCase):
         row = ['-1', 'guc_name', 'master_value', 'dbid']
         self.subject.update(FileSegmentGuc(row))
 
-        self.assertIn("Master  value: master_value | file: master_value", self.subject.report())
-        self.assertIn("Segment value: value | file: value", self.subject.report())
+        self.assertIn("Master  value: master_value | file: master_value\nSegment value: value | file: value", self.subject.report())
 
     def test_when_file_value_empty_file_compare_succeeds(self):
         row = ['0', 'guc_name', None, 'dbid']
@@ -98,8 +97,19 @@ class GucCollectionTest(GpTestCase):
         row = ['-1', 'guc_name', None, 'dbid']
         self.subject.update(FileSegmentGuc(row))
 
-        self.assertIn("Master  value: master_value | file: -", self.subject.report())
-        self.assertIn("Segment value: value | file: -", self.subject.report())
+        self.assertIn("Master  value: master_value | not set in file", self.subject.report())
+        self.assertIn("Segment value: value | not set in file", self.subject.report())
+
+    def test_values_unset_in_file_only(self):
+        self.subject = GucCollection() # remove existing DatabaseSegmentGucs
+
+        row = ['0', 'guc_name', None, 'dbid']
+        self.subject.update(FileSegmentGuc(row))
+        row = ['-1', 'guc_name', None, 'dbid']
+        self.subject.update(FileSegmentGuc(row))
+
+        self.assertIn("No value is set on master", self.subject.report())
+        self.assertIn("No value is set on segments", self.subject.report())
 
     def test_when_multiple_dbids_per_contentid_reports_failure(self):
         row = ['-1', 'guc_name', 'master_value', '1']
@@ -197,7 +207,7 @@ class GucCollectionTest(GpTestCase):
         row = ['0', 'guc_name', 'value', 'dbid4']
         self.subject.update(FileSegmentGuc(row))
 
-        self.assertIn("[context: -1] [dbid: dbid1] [name: guc_name] [value: -]\n", self.subject.report())
+        self.assertIn("[context: -1] [dbid: dbid1] [name: guc_name] [not set in file]\n", self.subject.report())
         self.assertIn("[context: -1] [dbid: dbid3] [name: guc_name] [value: master_value]\n", self.subject.report())
         self.assertIn("[context: 0] [dbid: dbid2] [name: guc_name] [value: value]\n", self.subject.report())
         self.assertIn("[context: 0] [dbid: dbid4] [name: guc_name] [value: value]", self.subject.report())
@@ -217,7 +227,7 @@ class GucCollectionTest(GpTestCase):
         row = ['1', 'guc_name', 'value', 'dbid5']
         self.subject.update(FileSegmentGuc(row))
 
-        self.assertEquals("[context: -1] [dbid: dbid1] [name: guc_name] [value: -]\n"
+        self.assertEquals("[context: -1] [dbid: dbid1] [name: guc_name] [not set in file]\n"
                           "[context: -1] [dbid: dbid3] [name: guc_name] [value: master_value]\n"
                           "[context: 0] [dbid: dbid2] [name: guc_name] [value: value]\n"
                           "[context: 0] [dbid: dbid4] [name: guc_name] [value: value]\n"