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

Add ssh url scheme handling #2516

Merged
merged 22 commits into from
Feb 12, 2018
Merged

Add ssh url scheme handling #2516

merged 22 commits into from
Feb 12, 2018

Conversation

cyrilico
Copy link
Contributor

@cyrilico cyrilico commented Dec 3, 2017

Fixes #2039
PR is ready to be merged

Tested and working in macOS.
App also sets itself as the default ssh protocol client (only need to change defaultSSHClient boolean in app/config/config-default.js if user desires otherwise).
The URL components are retrieved using some simple regular expressions, it currently supports URLs of the type ssh://[username@]hostname[:port][[/]initial/remote/path]

Here is a short video showing the feature working: https://www.youtube.com/watch?v=gprDu_8n0KM

Note: Although it appears as if package.json was heavily modified, only a few small changes were made, however indentation issues made git recognise it as otherwise

@antonioalmeida
Copy link

Travis CI's check failed because it timed out after 20 minutes downloading a file at very slow speeds, the previous commit passed without problems

@diogotorres97
Copy link

Probably this error is related with Travis CI Status

@cyrilico
Copy link
Contributor Author

cyrilico commented Dec 4, 2017

@albinekb I believe all is ok now (once again)

Copy link
Contributor

@albinekb albinekb left a comment

Choose a reason for hiding this comment

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

Very 💅 nice, @chabou can you have a look? I think this is ready to merge

@chabou
Copy link
Collaborator

chabou commented Dec 7, 2017

I must test it against HyperCLI and test if we can add multiple URL schemes.
I will do it ASAP

app/index.js Outdated
@@ -172,6 +172,11 @@ app.on('ready', () =>
// expose to plugins
app.createWindow = createWindow;

// check if should be set as default ssh protocol client
if (config.getConfig().defaultSSHApp == true && !app.isDefaultProtocolClient('ssh')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just do config.getConfig().defaultSShApp no need to test if true or else if you need to implicitly test it you will need to config.getConfig().defaultSShApp === true

Choose a reason for hiding this comment

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

Yup, my bad, just fixed it

@diogotorres97
Copy link

diogotorres97 commented Jan 22, 2018

@chabou @albinekb What is the current situation?

Copy link
Contributor

@albinekb albinekb left a comment

Choose a reason for hiding this comment

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

LGTM

@diogotorres97
Copy link

@albinekb rebase done!

Copy link
Collaborator

@chabou chabou left a comment

Choose a reason for hiding this comment

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

Thank you for your help 🙏. But there are some little things to change.


let userRegExp = /ssh:\/\/((?:\w+@)?[\w.]+)(?=:.*)?/;
let portRegExp = /.*:(\d+)(?=\/.*)?/;
let pathRegExp = /ssh:\/\/.*:(?:\d+)?(\/?.*)/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm think these regexp don't work.
For example, path is only matched if port is given:

'ssh://user@example.com:8080/tmp' -> 'ssh user@example.com -p 8080 -t "/tmp; bash"'
'ssh://user@example.com/tmp' -> 'ssh user@example.com'

We maybe should use something like: https://www.npmjs.com/package/parse-url

Choose a reason for hiding this comment

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

Yes, the regexp doesn't work on that case. I'll look into that package, but it's probably an overkill for this usage alone.


let command = 'ssh ' + user;
if (port != '') command += ' -p ' + port;
if (path != '') command += ' -t "' + path + '; bash"';
Copy link
Collaborator

Choose a reason for hiding this comment

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

two things here:
I think that you forget to put cd before adding path.

But we shouldn't force remote shell to bash. I don't think that there is an elegant solution to change remote directory at start. Path should be ignored imo (like iTerm2 from my testing).

Choose a reason for hiding this comment

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

Yeah, this is an ugly workaround, better to ignore paths.

@antonioalmeida
Copy link

Update:

@chabou
Copy link
Collaborator

chabou commented Feb 5, 2018

@antonioalmeida I added 2 commits:

  • First one to remove Hyper from default client for ssh:// protocol when user change its config.
  • Second one to deal with parse-url dependency. We only use yarn (with pinned dependency) and its yarn.lock lockfiles. And we need to add package to app/package.json when it is used by main process.

It works great. Thank you so much 🙏 . I'll merge it ASAP.

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.

7 participants