Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow options for custom ssh port #13

Merged
merged 8 commits into from
Apr 23, 2013
Merged

Allow options for custom ssh port #13

merged 8 commits into from
Apr 23, 2013

Conversation

PhilETaylor
Copy link
Contributor

Closes #12

Where custom portnumber is 5858 - This pull request allows

tugboat ssh fuzz -p 5858
tugboat ssh fuzz --ssh-port=5858

-p, [--ssh-port=SSH_PORT] # The custom SSH Port to connect to

It also allows a global port number configured in ~/.tugboat

Woohoo! My life is now complete :-)

@pearkes
Copy link
Collaborator

pearkes commented Apr 22, 2013

Nice! This looks great. Just a few things I think we can do to reduce the amount of code - mostly taking advantage of cool things in Thor. I'll make comments in the diffs.

@@ -58,11 +58,16 @@ def images
:type => :string,
:aliases => "-n",
:desc => "The exact name of the droplet"
method_option "ssh_port",
:type => :string,
:aliases => "-p",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add a default here (see tugboat create for an example of that syntax) then we can let Thor handle the default, instead of doing it ourselves.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - I see why you did it this way, actually. We need the user to be able to override the SSH port per command. Good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup - its great to have a default per tugboat installation, but would be even better to be able to have each droplet with their own pre-configured port number - my ruby was not up to that - so I went for being able to specify on the tugboat ssh fuzz -p 5858 command instead

@PhilETaylor
Copy link
Contributor Author

Thanks for your kind comments. I prefix this whole conversation again with the fact I'm no ruby developer - I'm a PHP guru, but at the end of the day I just copied and pasted your code examples and made this all work for me in an hour.

I like the thought of having a global ~/.tugboat ssh port number, as all my servers have the same non standard port (E.g. 5858 (Its not that ;-)) - but there are other times I might want to drop back to 22 on a certain command (maybe for a test droplet server) - so being able to specify on the command is great too.

The only bit missing that is the holy grail I think is to be able to set a default ssh port per droplet, so each droplet could be ssh'ed with a different port number without having to specify it on the command line - however thats is over and above the features in tugboat already (and therefore I had no clue how to implement it) as tugboat doesnt AFAIK store per-droplet configs anywhere yet.

Hope all this is helpful - I'm really scratching my own itch as I needed this :-)

@pearkes
Copy link
Collaborator

pearkes commented Apr 23, 2013

Great! It's definitely helpful, and don't worry at all about not being a Ruby guru. I'm personally more comfortable with Python. :)

So, my final nitpick, just to make this understandable in the long term, is the following:

Make the default ssh port (22) a constant, like the path.

Even though it's a little redundant, just making something like this:

ssh_port = DEFAULT_SSH_PORT

Go right below the other 2 here.

This just makes it super clear what we're doing. :)

Thanks a ton! Excited you're getting use out of the tool and are taking the time to bring your changes back upstream. After the changes above this will be good to merge. 👍

@PhilETaylor
Copy link
Contributor Author

ok I think I have done as requested :-) Please test.

@PhilETaylor
Copy link
Contributor Author

I have removed my ~/.tugboat and tried to authorize (with fake details) and I get a bug - this one is beyond my ruby debug skills :-( (Everything works with a valid ~/.tugboat in place, just not when starting from scratch with no config)

phils-imac-2012:tugboat phil$ bundle exec tugboat authorize
Faraday: you may want to install system_timer for reliable timeouts
Note: You can get this information from digitalocean.com/api_access

Enter your client key: fir
Enter your API key: rei
Enter your SSH key path (optional, defaults to ~/.ssh/id_rsa):
Enter your SSH user (optional, defaults to phil): fi
/Users/phil/Sites/tugboat/lib/tugboat/config.rb:47:in ssh_port': undefined method[]' for nil:NilClass (NoMethodError)
from /Users/phil/Sites/tugboat/lib/tugboat/config.rb:77:in create_config_file'
from /Users/phil/Sites/tugboat/lib/tugboat/middleware/ask_for_credentials.rb:14:incall'
from /Users/phil/Sites/tugboat/lib/tugboat/middleware/inject_configuration.rb:11:in call'
from /Library/Ruby/Gems/1.8/gems/middleware-0.1.0/lib/middleware/runner.rb:31:incall'
from /Library/Ruby/Gems/1.8/gems/middleware-0.1.0/lib/middleware/builder.rb:102:in call'
from /Users/phil/Sites/tugboat/lib/tugboat/cli.rb:32:inauthorize'
from /Library/Ruby/Gems/1.8/gems/thor-0.18.1/lib/thor/command.rb:27:in send'
from /Library/Ruby/Gems/1.8/gems/thor-0.18.1/lib/thor/command.rb:27:inrun'
from /Library/Ruby/Gems/1.8/gems/thor-0.18.1/lib/thor/invocation.rb:120:in invoke_command'
from /Library/Ruby/Gems/1.8/gems/thor-0.18.1/lib/thor.rb:363:indispatch'
from /Library/Ruby/Gems/1.8/gems/thor-0.18.1/lib/thor/base.rb:439:in start'
from /Users/phil/Sites/tugboat/bin/tugboat:4
from /Library/Ruby/Gems/1.8/bin/tugboat:19:inload'
from /Library/Ruby/Gems/1.8/bin/tugboat:19

Any pointers? Ideas?

@pearkes
Copy link
Collaborator

pearkes commented Apr 23, 2013

Ah, yes. We're not passing in the ssh_port into that function as an argument, see here.

That's fine though - we shouldn't be. Fixing this is as simple as removing the if check - it won't exist, because it's not in the authorize flow (as we decided previously), which is where we use create_config_file. Does that make sense?

@PhilETaylor
Copy link
Contributor Author

I'm adding the question to allow someone to set their default in authorize - this also fixes this - one mo

@PhilETaylor
Copy link
Contributor Author

Thats better - I can now authorize correctly and also the default is set to 22 if the question is not answered - probably for the best this way even though we decided against it in the early days

@PhilETaylor
Copy link
Contributor Author

Ive broken the travis build though :-) No idea how to fix that :-)

@pearkes
Copy link
Collaborator

pearkes commented Apr 23, 2013

Basically, because we changed the number of arguments in create_config_file, we'll need to update that everywhere in the tests.

Because there are some merge conflicts I'll pull this down and fix it up - no worries. Sorry, just added a ton of tests that have kludged up some of the stuff you worked on.

@PhilETaylor
Copy link
Contributor Author

No worries

@pearkes pearkes merged commit 16ac0b3 into petems:master Apr 23, 2013
pearkes added a commit that referenced this pull request Apr 23, 2013
@pearkes
Copy link
Collaborator

pearkes commented Apr 23, 2013

There we are - horrah! Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow configurable SSH port
2 participants