[RFC PATCH v5] add JSON output to net ads

Andrew Bartlett abartlet at samba.org
Fri Aug 24 00:37:41 UTC 2018


On Wed, 2018-08-22 at 13:12 +0200, Philipp Gesang wrote:
> -<| Quoting Andrew Bartlett <abartlet at samba.org>, on Wednesday, 2018-08-22 09:12:30 AM |>-
> > On Tue, 2018-08-21 at 08:29 +0200, Philipp Gesang via samba-technical
> > wrote:
> > > Hey guys,
> > > 
> > > this iteration addresses the comments to v4 by Andrew Bartlett
> > > (tests, merge conflicts).
> > > 
> > > CI: https://gitlab.com/phgsng/samba/pipelines/28267867
> > > 
> > > A note on the tests: Instead of making the code that parses the
> > > non-JSON output more permissive I chose to make the output of
> > > “net ads lookup” more regular. As a consequence, The blackbox
> > > tests now depend on this change. I’m aware that this change is
> > > not entirely related to the patch series. If it is not acceptable
> > > let me know and I’ll tweak the test code accordingly.
> > 
> > G'Day Philipp,
> > 
> > I'm still stumped as to how your patches differ from the ones that Gary
> > had where we just couldn't make --json work.  
> 
> No idea, sorry.
> 
> > Can you push it to the https://gitlab.com/samba/devel/samba repo so we
> > can see the full test run please?  
> 
> You mean https://gitlab.com/samba-team/devel/samba ?
>                             ^^^^^^^^^^
> Done: https://gitlab.com/samba-team/devel/samba/pipelines/28429561

Thanks.

> > Can you please ensure your tests catch exceptions.  Allowing
> > BlackboxProcessError seems quite wrong, that is exactly the type of
> > error you should be looking for.
> 
> Ok, will do. I saw other tests wrapped to catch
> BlackboxProcessError and assumed it signified a state where
> something was wrong with the harness or environment.

Sort of.  Yes, catching BlackboxProcessError is a good thing to do, but
you must raise an proper selftest failure in that case.

> > Finally if you run this against chgdcpass rather than the main
> > ad_dc_ntvfs and ad_dc environments you will have it run in your more
> > limited CI environment.  In a comment just explain that as this does
> > not log in, we can spread the load and run it against another
> > environment. 
> > (Only the build_samba_ad_dc_2 runs in private repos, sadly the rest of
> > the AD DC tests don't fit in the smaller free VMs provided by
> > gitlab.com). 
> 
> Ok, will do.

Finally, please don't use 'j', just have a long option --json.  Scripts
parsing JSON don't mind having to type extra characters and this avoids
issues with ambigious short options.

Thanks!

Andrew Bartlett

-- 
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT   
https://catalyst.net.nz/services/samba







More information about the samba-technical mailing list