[PATCH] CSV output for "samba-tool drs showrepl"

[Douglas Bagnall]

hi Flavio,

On 26/10/18 3:08 AM, Flavio Stanchina via samba-technical wrote:
> Andrew Bartlett via samba-technical wrote:
>> On Wed, 2018-10-24 at 17:20 +0200, Flavio Stanchina via samba-technical
>> wrote:
>>> This patch adds CSV output (option --csv) to "samba-tool drs
>>> showrepl".
>>>
>>> The output is meant to be equivalent to:
>>>     repadmin /showrepl /csv
>>>
>>> We use this to monitor our Samba DCs with Check_MK; [...]
>>
>> Since Samba 4.9 we have a --json output, and we are moving to JSON as
>> our text-based interchange format.  While not matching the windows tool
>> you mention, it should help your use case.
> 
> Using CSV was easy because it didn't require changes on the monitoring
> server. Using JSON would be better of course, but I would also need to hack
> a new parser into Check_MK and that's beyond the time I have available.
> 
>> That isn't to say we absolutely couldn't have a --csv option (say if
>> there are common scripts around the windows tool), but I would be more
>> cautious as it it isn't as structured.  It would also need (reliable!)
>> tests added.
> 
> Don't know if there are any other tools using REPADMIN's CSV output; I
> could argue that it should be added to samba-tool anyway for one-to-one
> feature compatibility with Windows tools, but I can see why you don't want
> to add code nobody's using.
> 
>> If you still wanted to go forward with it then rebasing it on master
>> and writing those tests would be the first step.
> 
> Thanks for your comments, will look into rebasing and writing some tests (I
> hope I can figure out how to do that).
> 

For tests, start with something like this:

--- a/source4/torture/drs/python/samba_tool_drs_showrepl.py
+++ b/source4/torture/drs/python/samba_tool_drs_showrepl.py
@@ -122,6 +122,16 @@ class SambaToolDrsShowReplTests(drs_base.DrsBaseTestCase):
                                  r'\tServer DN name  : %s'
                                  r'\n' % (GUID_RE, DN_RE))

+    def test_samba_tool_showrepl_csv(self):
+        """Tests 'samba-tool drs showrepl --csv' command.
+        """
+        out = self.check_output("samba-tool drs showrepl %s %s --csv" %
+                                (self.dc1, self.cmdline_creds))
+        data = get_string(out)
+
+        # assert you have the right headers and plausible data
+
+
     def test_samba_tool_showrepl_json(self):
         """Tests 'samba-tool drs showrepl --json' command.
         """


You *don't* want to start asserting the data makes sense with regard to
what you think the replication state should be, because that way you make
flapping tests which samba-tool drs showrepl has enough of already.

Rebasing on master will not be entirely straightforward as the showrepl
code has been rearranged -- but it shouldn't be too hard because it was
rearranged with a view to making this kind of addition simpler.

cheers,
Douglas