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

Pavel Reichl preichl at redhat.com
Thu Nov 5 10:27:05 UTC 2015



On 11/05/2015 11:06 AM, Lukas Slebodnik wrote:
> On (05/11/15 10:52), Sumit Bose wrote:
>> 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
> There was a disagreement in the content of patch.

Really? I think I expressed an agreement with Sumit's proposal.

On (04/11/15 18:58), Pavel Reichl wrote:
> however I agree with you that 'generic service name' is not a bad idea at all


> IMHO "ssh" was good enough and nothing needed to be changed.
>
> But Pavel insisted on a change but there was disagreement about using "sshd".

I don't think I insisted:
On (04/11/15 13:14), Pavel Reichl wrote:
> But If you don't like the change feel free to drop it, it's no doubt a nitpick.

> So the oneliner was used for UNBLOCKING review and cut off unimportant
> discussion. Because we could spent many hours about proper name.

And yet here we are. :-(


More information about the sssd-devel mailing list