ckeygen doesn't provide a default for the key file

[twisted-trac]

	@therve reported
Trac ID	trac#6607
Type	enhancement
Created	2013-06-30 12:46:51Z
Branch	https://github.com/twisted/twisted/tree/ckeygen-default-file-6607-2

For example:

$ ckeygen -p
Enter file in which the key is (/home/therve/.ssh/id_rsa):

It shows a file in parenthesis, but doesn't actually use that as a default value if nothing is specified. It should.

Attachments:

• 6607.patch (1535 bytes) - added by Saurabh on 2013-10-09 03:47:30Z -
• 6607_2.patch (5052 bytes) - added by Saurabh on 2013-11-07 13:33:47Z -
• 6607_3.patch (5063 bytes) - added by Saurabh on 2013-11-09 04:52:25Z -

Searchable metadata

trac-id__6607 6607
type__enhancement enhancement
reporter__therve therve
priority__low low
milestone__ 
branch__branches_ckeygen_default_file_6607_2 branches/ckeygen-default-file-6607-2
branch_author__lvh__glyph lvh, glyph
status__new new
resolution__ 
component__conch conch
keywords__easy easy
time__1372596411000000 1372596411000000
changetime__1423033410714556 1423033410714556
version__None None
owner__ioggstream ioggstream
cc__z3p

[twisted-trac]

	sgrondahl set status to closed

Fixed and submitted pull request -- commit b321af5.

[twisted-trac]

	@exarkun set status to reopened

Hi sgrondahl. Please leave tickets open until the fix is actually accepted.

What is b321af5? Does it refer to a changeset somewhere on github?

[twisted-trac]

	@lvh commented

Hi Saurabh,

Thanks for working on this. This appears to be missing a test. I'm not entirely sure if the call to strip is necessary (but hey, it doesn't really hurt either). I think it'd probably be better on several lines, since the lines are longer than 78 now.

cheers
lvh

[twisted-trac]

	Saurabh set owner to Saurabh Saurabh set status to new

[twisted-trac]

	Saurabh removed owner

[twisted-trac]

	@lvh set owner to @lvh

[twisted-trac]

	@lvh set owner to Saurabh

Hi saurabh,

Thanks for working on this some more! This is starting to look pretty good :)

A few minor nitpicks:

• Please observe the coding standard, for example:
• There should be whitespace after, but not before a comma
• ckeygen.py L102 L123 L174
• test_ckeygen.py L83 L162 L392
• No spaces before and after = in keyword arguments
• raw_input should definitely be an implementation detail since it's for testing only, so please prefix it with a _
• I can see how default_file becoming public API is good (so that the caller can make educated guesses for non-*nix systems), but given the previous point it should probably come before _raw_input
• In the test, don't use a bare except but catch the exception classes you actually mean (or expect)
• The new "get a filename" behavior is identical in all three affected functions, so it should probably be refactored.
• The behavior is only tested for displayPublicKey, but it's a change for changePassPhrase and printFingerprint as well.

Thanks again, and I look forward to merging this :)

lvh

[twisted-trac]

	Saurabh removed owner

[twisted-trac]

	Saurabh commented

I have refactored the "get filename" part as suggested..and made other mentioned small changes.

[twisted-trac]

	@lvh set owner to @lvh

[twisted-trac]

	@lvh commented

(In [40615]) Branching to 'ckeygen-default-file-6607'

[twisted-trac]

	@lvh commented

Hi again :)

There are some twistedchecker failures:

************* Module twisted.conch.scripts.ckeygen
W9208:102,0:_getFilename: Missing docstring
C0103:102,17:_getFilename: Invalid name "default_file" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103:102,31:_getFilename: Invalid name "_raw_input" (should match ([a-z_][a-zA-Z0-9]*)$)
W9501:105,16:_getFilename: String formatting operations should always use a tuplefor non-mapping values
C0301:111,0: Line too long (82/79)
C0103:111,30:printFingerprint: Invalid name "default_file" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103:111,60:printFingerprint: Invalid name "_raw_input" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103:129,30:changePassPhrase: Invalid name "default_file" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103:129,60:changePassPhrase: Invalid name "_raw_input" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103:177,30:displayPublicKey: Invalid name "default_file" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103:177,60:displayPublicKey: Invalid name "_raw_input" (should match ([a-z_][a-zA-Z0-9]*)$)
************* Module twisted.conch.test.test_ckeygen
W9011: 71,0: Blank line contains whitespace
W9010:152,0: Trailing whitespace found in the end of line

exarkun has reminded me of the argument that trying to name arguments with an underscore prefix is suboptimal. The short version of that argument (as I understand it) is that because you can call arguments named with an underscore without realizing they're private, the effect of "private" is gone. For anything else (like class, function or method names), you have to type the underscore, so you know you're doing something internal. (I think that the underscore still has value for documentation purposes; from what I understand, JP disagrees.)

The solution to this would be a private class _CKeygenCommands or something with methods:

• _getFilename
• printFingerprint
• changePassPhrase
• displayPublicKey

For testing, you instantiate one of those with a fake raw_input. There would also be a global instance on the module level; then:

_theInstance = _CKeygenCommands(raw_input)
printFingerprint = _theInstance.printFingerprint
changePassPhrase = _theInstance.changePassPhrase
displayPublicKey = _theInstance.displayPublicKey

That way, public API doesn't change, but you still get your DI-able, testable thing.

cheers
lvh

[twisted-trac]

	@lvh removed owner

[twisted-trac]

	Saurabh set owner to Saurabh

[twisted-trac]

	ioggstream set owner to @wallrj

Fixing proposal with a different approach from the above.

1- parsing and raw_input is done inside the GeneralOption class

2- key generation is done in a single function using a di(ct)spatch-table to pick the right function

3- added parsing tests

ioggstream@7316fc0

[twisted-trac]

	@glyph commented

(In [43817]) Branching to ckeygen-default-file-6607-2.

[twisted-trac]

	@glyph commented

Branch from github integrated and committed to a branch, builds forced.

[twisted-trac]

	@glyph set owner to ioggstream

Hi ioggstream,

Thanks for picking this up. I am sorry about the long turnaround; we are, as always, endeavoring to improve round-trip-time for code reviews.

I've run your code through the buildbot and there are numerous issues that will need to be addressed; lots of coding-standard issues, some tests failures due to optional dependencies not being conditionally imported, and suchlike. Please have a look at the build results and fix the errors; when you think they're fixed re-submit for review and I will endeavor to run them again promptly.

In addition:

1. in test_examples you should probably just be using self.fail rather than raise FailTest.
2. You've left in some commented-out code. When you want to comment out some code, just delete it. Version control can always bring it back.
3. You can just populate a default type with a default specification in the Options class; there's no need to have procedural code to initialize it.

I hope you'll have some time to look at this again at some point; let me know if you're still interested.

Thanks again for your contribution!

[twisted-trac]

	ioggstream commented

Hi glyph,

don't worry for the timing and thank you very much for yours. I will find some time to accomplish the mission.

I saw the bot logs and I need some hints:

1. pylint: as my patch is regards only twisted.conch.ckeygen I'm supposed to take care only of the associated stuff, not on the all twisted.conch right? I don't want to mess with more files than necessary;
2. pylintrc: I'm installing twistedchecker, so I'll be able to lint...
3. commented code: got it, code removed, thx++;
4. optional dependencies / conditional imports: they are actually present in twisted/conch/ssh/keys.py which I didn't modify: what should I do?
5. test_examples file will be removed from my patch, as it's about twisted.names
6. Option class default values, iirc I need some procedural code to respect old behavior.

Thx again for your feedback,
R.

[twisted-trac]

	@glyph commented

Replying to ioggstream:

Hi glyph,

don't worry for the timing and thank you very much for yours. I will find some time to accomplish the mission.

I saw the bot logs and I need some hints:

1. pylint: as my patch is regards only twisted.conch.ckeygen I'm supposed to take care only of the associated stuff, not on the all twisted.conch right? I don't want to mess with more files than necessary;

The buildbot should only be reporting errors in your specific change. If not, it's a twistedchecker bug, not your problem - you just need to file the bug and link it here so that any irrelevant warnings will pass review. In this fix, don't even fix all the stuff in ckeygen, just make sure your code doesn't add any new warnings.

1. pylintrc: I'm installing twistedchecker, so I'll be able to lint...
2. commented code: got it, code removed, thx++;
3. optional dependencies / conditional imports: they are actually present in twisted/conch/ssh/keys.py which I didn't modify: what should I do?

I believe the issue is that you added a test which is not properly skipped?

1. test_examples file will be removed from my patch, as it's about twisted.names

Great.

1. Option class default values, iirc I need some procedural code to respect old behavior.

You do need it to respect the old behavior according to the relevant policy, but wouldn't adding a default do the same thing?

Thx again for your feedback,
R.

You're welcome; thank you for your contribution :).