Hello,
please see attached patch set which contains some refactoring which is in my opinion quite safe. It doesn't touch the recursive call of pam_reply() as I think that changing that would be more risky change.
Thanks!
On (10/04/15 10:36), Pavel Reichl wrote:
Hello,
please see attached patch set which contains some refactoring which is in my opinion quite safe. It doesn't touch the recursive call of pam_reply() as I think that changing that would be more risky change.
Patches cannot be applied on top of Sumit's changes. "[PATCHES] Add special PAM prompting for 2FA authentication"
His patches has higher priority. So we should defer review of this patches until his patches will be accepted.
LS
On 04/10/2015 10:47 AM, Lukas Slebodnik wrote:
On (10/04/15 10:36), Pavel Reichl wrote:
Hello,
please see attached patch set which contains some refactoring which is in my opinion quite safe. It doesn't touch the recursive call of pam_reply() as I think that changing that would be more risky change.
Patches cannot be applied on top of Sumit's changes. "[PATCHES] Add special PAM prompting for 2FA authentication"
His patches has higher priority. So we should defer review of this patches until his patches will be accepted.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Rebased patch set is attached.
On (11/05/15 17:36), Pavel Reichl wrote:
Rebased patch set is attached.
Code coverage of function pam_reply was quite high but it covered just part which was necessary for Sumit's work.
I thought you would increase code coverage to all case you will change. and not just rebase.
I would prefer to spend two hours with reviewing coverage of unit test and then just check patches instead of checking all corner cases which could be result of moving code from one function to another.
The function set_delayed_response is not covered at all. and function add_warning_about_expiration covers very small potion of code.
Sort version: NACK without unit tests.
LS
On Mon, May 11, 2015 at 06:28:35PM +0200, Lukas Slebodnik wrote:
On (11/05/15 17:36), Pavel Reichl wrote:
Rebased patch set is attached.
Code coverage of function pam_reply was quite high but it covered just part which was necessary for Sumit's work.
I thought you would increase code coverage to all case you will change. and not just rebase.
I would prefer to spend two hours with reviewing coverage of unit test and then just check patches instead of checking all corner cases which could be result of moving code from one function to another.
The function set_delayed_response is not covered at all. and function add_warning_about_expiration covers very small potion of code.
Sort version: NACK without unit tests.
I need these patches (or their variant, still a bit of WIP) to test the PAM module properly, or rather to reuse the packing and unpacking. Please let me know if they've changed substantially since the original submission, but in the meantime I'll rebase and massage them in my pwrap branch (I push it to fedorapeople git occasionally)..
We can see about test coverage when my PAM module tests and your PAM responder tests materialize..
On 10/16/2015 09:30 AM, Jakub Hrozek wrote:
On Mon, May 11, 2015 at 06:28:35PM +0200, Lukas Slebodnik wrote:
On (11/05/15 17:36), Pavel Reichl wrote:
Rebased patch set is attached.
Code coverage of function pam_reply was quite high but it covered just part which was necessary for Sumit's work.
I thought you would increase code coverage to all case you will change. and not just rebase.
I would prefer to spend two hours with reviewing coverage of unit test and then just check patches instead of checking all corner cases which could be result of moving code from one function to another.
The function set_delayed_response is not covered at all. and function add_warning_about_expiration covers very small potion of code.
Sort version: NACK without unit tests.
I need these patches (or their variant, still a bit of WIP) to test the PAM module properly, or rather to reuse the packing and unpacking. Please let me know if they've changed substantially since the original submission, but in the meantime I'll rebase and massage them in my pwrap branch (I push it to fedorapeople git occasionally)..
We can see about test coverage when my PAM module tests and your PAM responder tests materialize.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I haven't done any changes on this patch set since the last submission.
But I'm really happy if you find them useful.
On Mon, May 11, 2015 at 06:28:35PM +0200, Lukas Slebodnik wrote:
On (11/05/15 17:36), Pavel Reichl wrote:
Rebased patch set is attached.
Code coverage of function pam_reply was quite high but it covered just part which was necessary for Sumit's work.
I thought you would increase code coverage to all case you will change. and not just rebase.
I would prefer to spend two hours with reviewing coverage of unit test and then just check patches instead of checking all corner cases which could be result of moving code from one function to another.
The function set_delayed_response is not covered at all. and function add_warning_about_expiration covers very small potion of code.
Sort version: NACK without unit tests.
This thread is deprecated in favor of "[PATCH] Unit tests for pam_sss using pam_wrapper".
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Reichl