[SSSD] [PATCH] TESTS: extend PAM responder unit test

Sumit Bose sbose at redhat.com
Thu Nov 5 09:52:35 UTC 2015


On Thu, Nov 05, 2015 at 10:29:41AM +0100, Lukas Slebodnik wrote:
> On (05/11/15 09:50), Pavel Reichl wrote:
> >
> >
> >On 11/05/2015 08:59 AM, Lukas Slebodnik wrote:
> >>On (04/11/15 18:58), Pavel Reichl wrote:
> >>>
> >>>
> >>>On 11/04/2015 06:41 PM, Sumit Bose wrote:
> >>>>On Wed, Nov 04, 2015 at 06:00:34PM +0100, Pavel Reichl wrote:
> >>>>>
> >>>>>
> >>>>>On 11/04/2015 03:02 PM, Lukas Slebodnik wrote:
> >>>>>>On (04/11/15 14:40), Pavel Reichl wrote:
> >>>>>
> >>>>>>>
> >>>>>>>Also one of few not yet tested corners of pam_reply() - 'Printing account
> >>>>>>>expiration warning for sshd' requires pam_service to be sshd, but I can
> >>>>>>>change when I will write another test (it might happen soon as pcech is
> >>>>>>>working on similar change in adjacent code).
> >>>>>>>
> >>>>>>That's exactly the reason why the default value should not be sshd.
> >>>>>
> >>>>>I don't understand why this would be the reason can you elaborate?
> >>>>
> >>>>Because a default value should not lead to a code path which handles a
> >>>>special case. The general PAM responder test should not run through the
> >>>>'sshd' case in pam_reply() only if the service is set explicitly to
> >>>>'sshd' this features should be tests.
> >>>
> >>>Sure, but just setting sshd would not lead to execution of this branch AFAIK
> >>>at least pam verbosity would be needed to be set. As said before service
> >>>value doesn't matter in current test so IMO this patch would only set service
> >>>to more sane default however I agree with you that 'generic service name' is
> >>>not a bad idea at all.
> >>>
> >>We spent enormous time with such unimportant patch.
> >>
> >>I pushed patch with Sumit's propsal "pam_test_service"
> >>using one-liner rule.
> >
> >So this rule really allows you to:
> >
> >1) amend patch yourself,
> >2) change authorship of the patch,
> Sumit proposed "pam_test_service" so he is author.

Pavel proposed to change the service name first.

> 
> >3) ack the patch yourself,
> yes
> 
> >4) push the patch yourself,
> yes
> 
> But only for oneliners

yes, this rule exists but imo it should be only used to un-break build
of to fix other urgent issues [*] and not to cut a discussion short.
Especially since Pavel already said that he like the idea of a generic
name and to me it sounded like he will include this in the next version
of his patch set.

bye,
Sumit

[*] I haven't checked all commits where this rule was used and I'm
pretty sure there will be examples where only a typo is fixed.
Nevertheless I think the rule is there to allow the swift handling of
emergencies and not to make it easy to commit minor change in general.
> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


More information about the sssd-devel mailing list