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

fix: Added default port ssh port 22 and forwarding of config port setting #6428

Merged
merged 5 commits into from
Sep 22, 2022

Conversation

Josh-Walker-GM
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM commented Sep 19, 2022

Related to #6426.

I have added in the default port option to the default server config DEFAULT_SERVER_CONFIG and have ensured the configured port setting is passed through to the node-ssh connection.

This is my first open-source contribution so thanks for your patience with me.

Release Notes

When doing a Baremetal deploy, you can now specify a port other than 22 to connect to via SSH. In your deploy.toml file just add the port key:

  [[production.servers]]
  host = "server.com"
+ port = 22022
  username = "user"
  agentForward = true
  sides = ["api","web"]
  path = "/var/www/app"
  processNames = ["serve"]
  repo = "git@github.com:myorg/myapp.git"

@Josh-Walker-GM
Copy link
Collaborator Author

Might be useful to mention @cannikin here, who helped me on the forum and created the issue.

@Josh-Walker-GM Josh-Walker-GM changed the title Added default port ssh port 22 and forwarding of config port setting fix: Added default port ssh port 22 and forwarding of config port setting Sep 19, 2022
@cannikin
Copy link
Member

Hey @Josh-Walker-GM Congrats on your first open source contribution!!

If you could update the docs as well that’d be great. You can find that doc here: https://github.com/redwoodjs/redwood/blob/main/docs/docs/deploy/baremetal.md If you want to test out your changes you can run yarn start from the /docs dir and then when the site comes up, change the drop-down at top left to be “Canary”.

@cannikin cannikin added topic/deployment release:feature This PR introduces a new feature labels Sep 19, 2022
@cannikin cannikin assigned cannikin and unassigned dac09 Sep 19, 2022
@Josh-Walker-GM
Copy link
Collaborator Author

Thanks @cannikin, not a major contribution but happy to help.

I've added the required documentation.

I notice the only other node-ssh config option which is not handled here is the tryKeyboard - which I believe simply requests the credentials interactively when enabled? Is this something which has been consciously omitted?

@cannikin
Copy link
Member

I notice the only other node-ssh config option which is not handled here is the tryKeyboard - which I believe simply requests the credentials interactively when enabled? Is this something which has been consciously omitted?

Wellllll I don't think our CLI interface easily handles interactive keyboard...none of our existing CLI commands do it so we've never really tried. We have a couple that use prompt to ask for a single letter and then do something after, but never had to accept input and then forward it on to something else. You're welcome to try to add it if you'd like! But we could keep this PR focused on just the port and start a new one for that option if you think you want to add it!

@Josh-Walker-GM
Copy link
Collaborator Author

Okay I might have a quick look and see if it turns into a big can of worms or not but I agree best to keep the PR as focused as possible.

I believe I'll need to update some of the tests. I noticed one failed earlier as it does not like having the additional port option present.

@cannikin
Copy link
Member

Sorry for the radio silence! More meetings and travel today, but I’ll be back at home tomorrow and can review.

@cannikin cannikin merged commit 8d3f60f into redwoodjs:main Sep 22, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Sep 22, 2022
@cannikin
Copy link
Member

Thanks again for the quick turnaround on this one, and congrats on your first open source PR! 🎉

@Josh-Walker-GM
Copy link
Collaborator Author

My pleasure, was an interesting experience. I hope to perhaps contribute more even if I am a little new to a project like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature topic/deployment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants