[PATCHES] samba-tool: implement user show command to display a user AD object

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
Hi!

Please find patches attached which add a samba-tool user show sub
command. The new command allows to display a user AD object or just the
specified object attributes, like the dn or description.

The second patch provides a test for the new command.

The third patch fixes some typos. This is not my native language, but
according to the rules i know, it should be "a user" and not "an user".

Please review and push or let me know what you think about the patches.

Thanks,
Björn

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

samba-tool_user-show.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
On Fri, 8 Dec 2017 12:40:25 +0100
Bjoern Baumbach via samba-technical <[hidden email]>
wrote:

> Hi!
>
> Please find patches attached which add a samba-tool user show sub
> command. The new command allows to display a user AD object or just
> the specified object attributes, like the dn or description.
>
> The second patch provides a test for the new command.
>
> The third patch fixes some typos. This is not my native language, but
> according to the rules i know, it should be "a user" and not "an
> user".
>
> Please review and push or let me know what you think about the
> patches.
>
> Thanks,
> Björn
>

I don't really understand the reason behind this patch, what does it
give you that an ldbsearch doesn't, where would you use it ?

There are also lines over 80 columns.
.
I would also think that the typos should be in separate patches.

Rowland

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
Hi Rowland,

thank you for the review.

On 12/08/2017 01:02 PM, Rowland Penny wrote:
> I don't really understand the reason behind this patch, what does it
> give you that an ldbsearch doesn't, where would you use it ?

I would like to introduce a user move command, later, to move a user
into an OU or a different container. With the show command you can
easily display the users DN and further attributes.
I assume that there are users which do not make use of the ldb-tools but
of the samba-tool. The usage is more easy and looks more generic for
them - some of the users are afraid of using the ldb commands directly.

Anyway, if you ask "what does it give you that an ldbsearch doesn't", we
could also remove commands like edit (can be done by ldbedit), list (can
be done by ldbsearch), delete (can be done by ldbdel), ...
But it's nice to have all the commands on one place where a user would
expect and like too see it.

> There are also lines over 80 columns.

I'll fix this.

> I would also think that the typos should be in separate patches.

It is already a separate patch. Or would you split the patch for some
reason?

Best regards,
Björn

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
On Fri, 8 Dec 2017 13:49:50 +0100
Bjoern Baumbach <[hidden email]> wrote:

> Hi Rowland,
>
> thank you for the review.
>
> On 12/08/2017 01:02 PM, Rowland Penny wrote:
> > I don't really understand the reason behind this patch, what does it
> > give you that an ldbsearch doesn't, where would you use it ?
>
> I would like to introduce a user move command, later, to move a user
> into an OU or a different container. With the show command you can
> easily display the users DN and further attributes.

Why not just create the move command, now this would be useful, i.e.
samba-tool user move username OU-to-move-to

Find the user, exit if not found
Check the user isn't already in the OU
then use samba.Ldb.rename

Of course you would possibly have to create the OU first:

samdb.create_ou
 

> I assume that there are users which do not make use of the ldb-tools
> but of the samba-tool. The usage is more easy and looks more generic
> for them - some of the users are afraid of using the ldb commands
> directly.
>
> Anyway, if you ask "what does it give you that an ldbsearch doesn't",
> we could also remove commands like edit (can be done by ldbedit),
> list (can be done by ldbsearch), delete (can be done by ldbdel), ...
> But it's nice to have all the commands on one place where a user would
> expect and like too see it.

well yes, but I cannot see the point in having something that just
gives you a DN and possibly other attributes. If you need the DN to do
something, you might as well write a script around ldbsearch and
ldbmodify etc. This is just my opinion however, others may think
differently.

>
> > There are also lines over 80 columns.
>
> I'll fix this.
>
> > I would also think that the typos should be in separate patches.
>
> It is already a separate patch. Or would you split the patch for some
> reason?

Yes, it is just one patch, but I think it should be separate patches,
your main patch does not rely on the typo patches and visa-versa

Rowland

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
On 12/08/2017 02:22 PM, Rowland Penny via samba-technical wrote:

> On Fri, 8 Dec 2017 13:49:50 +0100
> Bjoern Baumbach <[hidden email]> wrote:
>> On 12/08/2017 01:02 PM, Rowland Penny wrote:
>>> I don't really understand the reason behind this patch, what does it
>>> give you that an ldbsearch doesn't, where would you use it ?
>>
>> I would like to introduce a user move command, later, to move a user
>> into an OU or a different container. With the show command you can
>> easily display the users DN and further attributes.
>
> Why not just create the move command, now this would be useful, i.e.
> samba-tool user move username OU-to-move-to
>
> Find the user, exit if not found
> Check the user isn't already in the OU
> then use samba.Ldb.rename
I already did it in a similar way. I'll provide the patches as soon I've
added a proper test.
But I just mentioned it as an example, that you might want to check the
current OU of the user before you move the user.

>> I assume that there are users which do not make use of the ldb-tools
>> but of the samba-tool. The usage is more easy and looks more generic
>> for them - some of the users are afraid of using the ldb commands
>> directly.
>>
>> Anyway, if you ask "what does it give you that an ldbsearch doesn't",
>> we could also remove commands like edit (can be done by ldbedit),
>> list (can be done by ldbsearch), delete (can be done by ldbdel), ...
>> But it's nice to have all the commands on one place where a user would
>> expect and like too see it.
>
> well yes, but I cannot see the point in having something that just
> gives you a DN and possibly other attributes. If you need the DN to do
> something, you might as well write a script around ldbsearch and
> ldbmodify etc. This is just my opinion however, others may think
> differently.
Yes of course, I appreciate that you tell me your opinion! :-)

As I said, I think it's nice to have a small command to request the user
information which can be easily used for all users.

>>> There are also lines over 80 columns.
>>
>> I'll fix this.
I've attached a new patch.
>>> I would also think that the typos should be in separate patches.
>>
>> It is already a separate patch. Or would you split the patch for some
>> reason?
>
> Yes, it is just one patch, but I think it should be separate patches,
> your main patch does not rely on the typo patches and visa-versa

Sorry, but I do not understand what you mean. The typos are all in the
same code, why should I separate them? Or do you not like that I send
them all together in one file?

Best regards,
Björn

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

samba-tool_user-show.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
On Fri, 8 Dec 2017 16:32:40 +0100
Bjoern Baumbach <[hidden email]> wrote:

> On 12/08/2017 02:22 PM, Rowland Penny via samba-technical wrote:
> > On Fri, 8 Dec 2017 13:49:50 +0100
> > Bjoern Baumbach <[hidden email]> wrote:
> >> On 12/08/2017 01:02 PM, Rowland Penny wrote:
> >>> I don't really understand the reason behind this patch, what does
> >>> it give you that an ldbsearch doesn't, where would you use it ?
> >>
> >> I would like to introduce a user move command, later, to move a
> >> user into an OU or a different container. With the show command
> >> you can easily display the users DN and further attributes.
> >
> > Why not just create the move command, now this would be useful, i.e.
> > samba-tool user move username OU-to-move-to
> >
> > Find the user, exit if not found
> > Check the user isn't already in the OU
> > then use samba.Ldb.rename
>
> I already did it in a similar way. I'll provide the patches as soon
> I've added a proper test.
> But I just mentioned it as an example, that you might want to check
> the current OU of the user before you move the user.

You can do this with samba-tool user edit, but it is not really down to
just me, perhaps someone else will give their opinion.

>
> >> I assume that there are users which do not make use of the
> >> ldb-tools but of the samba-tool. The usage is more easy and looks
> >> more generic for them - some of the users are afraid of using the
> >> ldb commands directly.

I can understand this, but still cannot see what your patch gives Samba
what it doesn't already have ;-)

> >>
> >> Anyway, if you ask "what does it give you that an ldbsearch
> >> doesn't", we could also remove commands like edit (can be done by
> >> ldbedit), list (can be done by ldbsearch), delete (can be done by
> >> ldbdel), ... But it's nice to have all the commands on one place
> >> where a user would expect and like too see it.
> >
> > well yes, but I cannot see the point in having something that just
> > gives you a DN and possibly other attributes. If you need the DN to
> > do something, you might as well write a script around ldbsearch and
> > ldbmodify etc. This is just my opinion however, others may think
> > differently.
>
> Yes of course, I appreciate that you tell me your opinion! :-)
>
> As I said, I think it's nice to have a small command to request the
> user information which can be easily used for all users.
>
> >>> There are also lines over 80 columns.
> >>
> >> I'll fix this.
> I've attached a new patch.
> >>> I would also think that the typos should be in separate patches.
> >>
> >> It is already a separate patch. Or would you split the patch for
> >> some reason?
> >
> > Yes, it is just one patch, but I think it should be separate
> > patches, your main patch does not rely on the typo patches and
> > visa-versa
>
> Sorry, but I do not understand what you mean. The typos are all in the
> same code, why should I separate them? Or do you not like that I send
> them all together in one file?

Okay, the typos have nothing to do with your your proposed main patch,
they are 'repairing' a small problem, so should be in their own patch
and as such should be pushed, but your main patch will work without
them, so it should be in its own patch, which we could then discuss the
merits of ;-)

Rowland

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
Hi folks,

On Fri, Dec 08, 2017 at 03:49:45PM +0000, Rowland Penny via samba-technical wrote:

> > On 12/08/2017 02:22 PM, Rowland Penny via samba-technical wrote:
> > > On Fri, 8 Dec 2017 13:49:50 +0100
> > > Bjoern Baumbach <[hidden email]> wrote:
> > >> On 12/08/2017 01:02 PM, Rowland Penny wrote:
> > >>> I don't really understand the reason behind this patch, what does
> > >>> it give you that an ldbsearch doesn't, where would you use it ?
> > >>
> > >> I would like to introduce a user move command, later, to move a
> > >> user into an OU or a different container. With the show command
> > >> you can easily display the users DN and further attributes.
> > >
> > > Why not just create the move command, now this would be useful, i.e.
> > > samba-tool user move username OU-to-move-to
> > >
> > > Find the user, exit if not found
> > > Check the user isn't already in the OU
> > > then use samba.Ldb.rename
> >
> > I already did it in a similar way. I'll provide the patches as soon
> > I've added a proper test.
> > But I just mentioned it as an example, that you might want to check
> > the current OU of the user before you move the user.
>
> You can do this with samba-tool user edit, but it is not really down to
> just me, perhaps someone else will give their opinion.

I want the show command.

> > >> I assume that there are users which do not make use of the
> > >> ldb-tools but of the samba-tool. The usage is more easy and looks
> > >> more generic for them - some of the users are afraid of using the
> > >> ldb commands directly.
>
> I can understand this, but still cannot see what your patch gives Samba
> what it doesn't already have ;-)

A consistent samba-tool interface.

> > >>> I would also think that the typos should be in separate patches.
> > >>
> > >> It is already a separate patch. Or would you split the patch for
> > >> some reason?
> > >
> > > Yes, it is just one patch, but I think it should be separate
> > > patches, your main patch does not rely on the typo patches and
> > > visa-versa
> >
> > Sorry, but I do not understand what you mean. The typos are all in the
> > same code, why should I separate them? Or do you not like that I send
> > them all together in one file?
>
> Okay, the typos have nothing to do with your your proposed main patch,
> they are 'repairing' a small problem, so should be in their own patch
> and as such should be pushed, but your main patch will work without
> them, so it should be in its own patch, which we could then discuss the
> merits of ;-)

they are in their own patch. It's one patch*set*, but that's perfectly fine
adding related typo fix patches into such a patchset.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
On 09/12/17 05:24, Ralph Böhme via samba-technical wrote:
>
> I want the show command.

Me too.

I think our aim should be that people never need to use ldbsearch or
ldbedit in normal operations.

Our command line interfaces should be simple and clear and
comprehensive and not require *very* much knowledge of how Samba works
behind the scenes. As well as adding missing samba-tool commands, we
also need to make some of the existing ones shut up a bit and try to
present their message better.

Along these lines I am very slowly working on patches to make
samba-tool explain what the hell DRS replication is trying to do,
rather than dumping a strange collection of ldb objects.

I'd also like to introduce a --color={yes,no,auto} option, similar to
modern command line tools like "git" and "ls".

cheers,
Douglas

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
On 09/12/17 05:24, Ralph Böhme via samba-technical wrote:
> On 12/08/2017 11:19 PM, Douglas Bagnall via samba-technical wrote:
>> I want the show command.
> Me too.

Thank you very much for your feedback.

As I've had fixed the code concerning Rowlands comments, does someone
like to have a last look on the patches and push them if they are fine?

Thanks
Björn

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
On Wed, 2017-12-13 at 23:30 +0100, Bjoern Baumbach via samba-technical
wrote:

> On 09/12/17 05:24, Ralph Böhme via samba-technical wrote:
> > On 12/08/2017 11:19 PM, Douglas Bagnall via samba-technical wrote:
> > > I want the show command.
> >
> > Me too.
>
> Thank you very much for your feedback.
>
> As I've had fixed the code concerning Rowlands comments, does someone
> like to have a last look on the patches and push them if they are fine?

I'll look at this tomorrow and pick up any other stray patches I can
find.

I feel like the 4.8 rush has started well!

Andrew Bartlett

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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] samba-tool: implement user show command to display a user AD object

Samba - samba-technical mailing list
On 12/14/2017 11:28 AM, Andrew Bartlett wrote:
>> Thank you very much for your feedback.
>>
>> As I've had fixed the code concerning Rowlands comments, does someone
>> like to have a last look on the patches and push them if they are fine?
> I'll look at this tomorrow and pick up any other stray patches I can
> find.

Sorry, there was a typo in the commands description.
I've attached a new patch set.

Best regards,
Björn

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

user-show.patch (10K) Download Attachment