search

markolson / chef-ssh

Chef cookbook for managing some mildly-difficult-to-automate SSH configuration
39 stars 54 forks source link

Allow comments in authorized_keys files #63

Closed jochenseeber closed 7 years ago

jochenseeber commented 8 years ago

SSH authorizedkeys files may contain comments (lines starting with '#'). This PR adds handling for comment lines and also fixes an error with a missing parameter in validate*_option.

tejaycar commented 8 years ago

So, to be clear, this is just to prevent the cookbook from stripping out existing comment lines from an existing file?

jochenseeber commented 8 years ago

No, it is also because the current implementation crashes when an existing file contains comments. The solution keeps comments in the key file intact.

tejaycar commented 8 years ago

@jochenseeber sorry, I'm pretty swamped right now. Any chance you can add some tests for this MR?

voroniys commented 7 years ago

Please include this MR as soon as possible. You do not need tests for fixing a bug. Current version crashes in file contains comments and this is really annoying.

tejaycar commented 7 years ago

@voroniys I appreciate that you have an opinion regarding tests, but I do not merge bug fixes (or any other code) without tests that: A. reproduce the bug AND B. demonstrate that it is fixed

I do this for two reasons. First, sometimes a perceived bug isn't actually a bug. In writing the test, it often exposes the true nature of the bug (or in some cases a simple user error). Secondly, I want to ensure I don't re-introduce the bug at some future point, and a test will help protect against that. In fact, I recently make some changes to pull in another PR and they did, in fact, re-introduce a bug that another PR had fixed earlier that day. Because that fix contained a test, I was able to catch the regression immediately and fix it before releasing the code.

I am going to do what I can to reproduce this and provide a fix.

voroniys commented 7 years ago

I would understand your point about testing if it would be recent PR. This one is over half year old! And all this time your cookbook has a nasty bug. I would even say terrible one, because you've also got exception from exception - a double bug! And if you don't trust PR you can reject it - it is your project, but knowing about the bug and doing nothing about this for half year using absence of testing is extremely bad excuse.

Do you know what happens than? The people like me, who needs just working cookbook are using the version of @jochenseeber. It just works and I don't care has it proper testing or not.

And "I'm going to do what I can to reproduce" in this particular cases looks very funny - in half year time you have not found 1 minute of time to add to any authorized_keys file line like # this is a stupid comment that crashes my cookbook and run chef-client?

You did a very good job with this cookbook, don't spoil it now! Just find 15 minutes to fix this bug. This is exactly the time I spent for fixing it in the first place, before I went to look on github. But than I've found that here PR for fixing it is waiting for over 6 month for being accepted. Not nice at all.

And if you believe, that test are so important and essential spend another 15 minutes for making a test for it.

tejaycar commented 7 years ago

If this bug was causing you so much pain personally, perhaps you could have found 15 minutes to add a test as I asked? Alternately, remove the comment from you file. Welcome to open source software. I've got a day job, a family, and a host of other responsibilities. So, as with all other open source software, you are free to:

  1. Submit a PR that meets the guidelines of the project
  2. Fork it
  3. Re-write it
  4. Use something else

As it stands, I'm currently spending my own personal time to fix a bug that has not impacted me in the 5 years I've used this cookbook. Try to show a little gratitude for a cookbook you didn't have to write yourself.

On Fri, Feb 24, 2017 at 6:37 AM, Stanislav Voroniy <notifications@github.com

wrote:

I would understand your point about testing if it would be recent PR. This one is over half year old! And all this time your cookbook has a nasty bug. I would even say terrible one, because you've also got exception from exception - a double bug! And if you don't trust PR you can reject it - it is your project, but knowing about the bug and doing nothing about this for half year using absence of testing is extremely bad excuse.

Do you know what happens than? The people like me, who needs just working cookbook are using the version of @jochenseeber https://github.com/jochenseeber. It just works and I don't care has it proper testing or not.

And "I'm going to do what I can to reproduce" in this particular cases looks very funny - in half year time you have not found 1 minute of time to add to any authorized_keys file line like this is a stupid comment that crashes my cookbook

and run chef-client?

You did a very good job with this cookbook, don't spoil it now! Just find 15 minutes to fix this bug. This is exactly the time I spent for fixing it in the first place, before I went to look on github. But than I've found that here PR for fixing it is waiting for over 6 month for being accepted. Not nice at all.

And if you believe, that test are so important and essential spend another 15 minutes for making a test for it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/markolson/chef-ssh/pull/63#issuecomment-282292599, or mute the thread https://github.com/notifications/unsubscribe-auth/ADqwzCUIUe22xQi9CJN_ZvWy4831EHATks5rftz_gaJpZM4JG9wY .

tejaycar commented 7 years ago

fixed in v0.10.20

jochenseeber commented 7 years ago

Sorry, I was completely swamped and had no time to look into unit tests. However, thanks for the merge, and thanks for providing this cookbook.

And @voroniys, the people providing this cookbook have made a valuable contribution to the community. This is their house and their rules. You need not agree, but then you're always free to not use their work.

tejaycar commented 7 years ago

Thanks for the kind words Jochen. Believe me, I know what you mean about being swamped. Hence the long delay getting this done. Thankfully, I have things in working order again so any new fixes should be pretty quick.

On Thu, Mar 16, 2017 at 11:41 AM, Jochen Seeber notifications@github.com wrote:

Sorry, I was completely swamped and had no time to look into unit tests. However, thanks for the merge, and thanks for providing this cookbook.

And @voroniys https://github.com/voroniys, the people providing this cookbook have made a valuable contribution to the community. This is their house and their rules. You need not agree, but then you're always free to not use their work.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/markolson/chef-ssh/pull/63#issuecomment-287135853, or mute the thread https://github.com/notifications/unsubscribe-auth/ADqwzG6gJrujLNvrLCQmigUI3MteZlNqks5rmXQwgaJpZM4JG9wY .