Opened 4 years ago

Last modified 3 years ago

#6607 enhancement new

ckeygen doesn't provide a default for the key file

Reported by: therve Owned by: Roberto Polli
Priority: low Milestone:
Component: conch Keywords: easy
Cc: z3p Branch: branches/ckeygen-default-file-6607-2
branch-diff, diff-cov, branch-cov, buildbot
Author: lvh, glyph

Description

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 (3)

6607.patch (1.5 KB) - added by Saurabh 4 years ago.
6607_2.patch (4.9 KB) - added by Saurabh 4 years ago.
6607_3.patch (4.9 KB) - added by Saurabh 4 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 4 years ago by Samuel Grondahl

Resolution: fixed
Status: newclosed

Fixed and submitted pull request -- commit b321af5c0251e3ef0bfdd87cdd1d52470d07e4ee.

comment:3 Changed 4 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

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

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

Changed 4 years ago by Saurabh

Attachment: 6607.patch added

comment:4 Changed 4 years ago by Saurabh

Keywords: review added

comment:5 Changed 4 years ago by lvh

Keywords: review removed

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

comment:6 Changed 4 years ago by Saurabh

Owner: set to Saurabh
Status: reopenednew

Changed 4 years ago by Saurabh

Attachment: 6607_2.patch added

comment:7 Changed 4 years ago by Saurabh

Keywords: review added

comment:8 Changed 4 years ago by Saurabh

Owner: Saurabh deleted

comment:9 Changed 4 years ago by lvh

Keywords: review removed
Owner: set to lvh

comment:10 Changed 4 years ago by lvh

Owner: changed from lvh 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

Changed 4 years ago by Saurabh

Attachment: 6607_3.patch added

comment:11 Changed 4 years ago by Saurabh

Keywords: review added
Owner: Saurabh deleted

comment:12 Changed 4 years ago by Saurabh

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

comment:13 Changed 4 years ago by lvh

Owner: set to lvh

comment:14 Changed 4 years ago by lvh

Author: lvh
Branch: branches/ckeygen-default-file-6607

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

comment:15 Changed 4 years ago by lvh

Keywords: review removed

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

comment:16 Changed 4 years ago by lvh

Owner: lvh deleted

comment:17 Changed 4 years ago by Saurabh

Owner: set to Saurabh

comment:18 Changed 3 years ago by Roberto Polli

Keywords: review added
Owner: changed from Saurabh to Richard Wall

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

https://github.com/ioggstream/twisted/commit/7316fc008e22621e046aad8a2b8ede751c97ca7e

comment:19 Changed 3 years ago by Glyph

Author: lvhlvh, glyph
Branch: branches/ckeygen-default-file-6607branches/ckeygen-default-file-6607-2

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

comment:20 Changed 3 years ago by Glyph

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

comment:21 Changed 3 years ago by Glyph

Keywords: review removed
Owner: changed from Richard Wall to Roberto Polli

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!

comment:22 Changed 3 years ago by Roberto Polli

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.

comment:23 in reply to:  22 Changed 3 years ago by Glyph

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 :).

Note: See TracTickets for help on using tickets.