On Wed, 2015-04-08 at 17:53 +0200, Martin Basti wrote: > On 08/04/15 17:46, Luc de Louw wrote: > > On 04/08/2015 05:14 PM, Martin Basti wrote: > > > On 08/04/15 17:12, Luc de Louw wrote: > > > > > > > > On 04/08/2015 05:05 PM, Martin Basti wrote: > > > > > On 08/04/15 16:55, Nathaniel McCallum wrote: > > > > > > On Wed, 2015-04-08 at 11:57 +0200, Luc de Louw wrote: > > > > > > > Hi there, > > > > > > > > > > > > > > At the moment ipa otptoken-add-yubikey does not add the > > > > > > > parameter > > > > > > > "APPEND_CR". This prevents submit the password+OTP. > > > > > > > APPEND_CR is > > > > > > > usually > > > > > > > very handy, most people use this functionality. > > > > > > > > > > > > > > The patch changes the behavior to set APPEND_CR by > > > > > > > default and let > > > > > > > the > > > > > > > user override this by using the the --do-not-append-cr > > > > > > > option. > > > > > > This patch is very helpful and I would like to see it > > > > > > merged. Thanks > > > > > > Luc! > > > > > > > > > > > > 1. This patch needs to be formatted according to the > > > > > > FreeIPA > > > > > > formatting. See: > > > > > > https://www.freeipa.org/page/Contribute/Patch_Format > > > > > > > > > > > > 2. The flag should be named "no_cr" instead of > > > > > > "do_not_append_cr". > > > > > > > > > > > > 3. The comment is not necessary since what the code does > > > > > > is obvious. > > > > > > > > > > > > Nathaniel > > > > > > > > > > > Hello, > > > > > > > > > > 4) this patch changes API, so please run ./makeapi to > > > > > regenerate > > > > > API.txt > > > > > file and add changes into patch + please bum API minor > > > > > version in > > > > > VERSION file > > > > > > > > > > thanks. > > > > > > > > > > > > > > > > > Hi, > > > > > > > > When running makeaip, I get the following error: > > > > File "/home/luc/freeipa/ipalib/constants.py", line 25, in > > > > <module> > > > > from ipaplatform.paths import paths > > > > ImportError: No module named paths > > > > > > > > Any hints? > > > > > > > > The other changes are ready to submit. > > > > > > > > Thanks, > > > > > > > > Luc > > > You may need to run 'make version-upgrade' or 'make' to prepare > > > the > > > module. > > > > > > If it will not work, you can send incomplete patch, I will add > > > API > > > changes there, just bump VERSION please > > > > > > > Martin, > > > > Thanks for your hints, seems to work, please have a look at it... > > > > Thanks, > > > > Luc > > > > > Thanks, > > please change the comment too > > -IPA_API_VERSION_MINOR=116 > +IPA_API_VERSION_MINOR=117 > # Last change: tbordaz - Add stageuser_add command" > > Otherwise patch looks good, but Nathaniel is the OTP guru, he should > say > final ack.
I'm also a tough reviewer. :) 1. Remove the unnecessary code comment. 2. There appears to be inconsistent indentation in the flag parameter specification. It is probably a mix of tabs and spaces. 3. The git commit comment should contain one short summary line without terminating punctuation followed by any necessary explanatory paragraphs. You can change this via the "--amend" option to "git commit". Try the following: Enable YubiKey carriage return emission via otptoken-add-yubikey Before this patch, YubiKeys programmed by IPA would not emit the carriage return character at the end of the OTP value. This requires the user to press his YubiKey and then (unnecessarily) the Enter or Return key. After this patch, the user only needs to press the YubiKey. Should a user desire to omit the carriage return character, the --no- cr option can be specified. Nathaniel -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
