Dne 8.4.2015 v 22:52 Martin Kosek napsal(a):
On 04/08/2015 06:03 PM, Nathaniel McCallum wrote:
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


One more note to the API. By my experience, using a Flag for a boolean
data input has often proved to be a bad call.

Let's say you now introduce --no-cr flag. What if we decide to change
the default to False? How would you then change the option/API?

You would have to add --cr flag.


It is more flexible IMO to just use something like

--cr=TRUE|FALSE with TRUE being the default

I would say --append-cr=TRUE|FALSE with no default, meaning do not add the flag to the config at all.


Martin



--
Jan Cholasta

--
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

Reply via email to