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 the option to pass in a custom port #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lsaether
Copy link

@lsaether lsaether commented May 27, 2020

We run ssh on a custom port so we needed a way to tell cargo remote to use that port instead. I don't usually write Rust so I don't know if this is the most ergonomic solution. Opening mostly for awareness.

I also ran cargo fmt which formatted some of the lines that I did not change.

@sgeisler
Copy link
Owner

Will have a closer look at it later and probably merge. But wouldn't it be easier to write a ssh config entry for the build host?

Copy link
Owner

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

It would be nice if you could change the "if None, then empty string" statements to a more idiomatic .args(port.map(|p| format!("-p {}", p))).

.arg(if port.is_some() {
format!("-e ssh -p {}", port.clone().unwrap())
} else {
"".to_string()
Copy link
Owner

Choose a reason for hiding this comment

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

That's a bit of an ugly hack. You could use .args() here, which accepts T: IntoIterator<…>. Option does implement IntoIterator (just map the port).

long = "port",
help = "Custom port for ssh on the build server"
)]
port: Option<String>,
Copy link
Owner

Choose a reason for hiding this comment

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

This should be Option<u16>, a String allows too many invalid inputs. This would also make port Copy, avoiding al these clone()s.

@sgeisler sgeisler mentioned this pull request Jul 13, 2020
@TheAlgorythm
Copy link
Contributor

Wouldn't it be better if the port is stored in the config?
Would you accept a PR where I deserialize the config with serde?

@sgeisler
Copy link
Owner

@TheAlgorythm tbh I had totally forgotten there was a config file. You are right that that is probably the better place for it. I'm still not sure why you wouldn't just define a host in your ssh config though as cargo remote will probably never support all ssh options.

Would you accept a PR where I deserialize the config with serde?

Yes, definitely, I'm not sure why I didn't do it this way back then tbh.

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.

None yet

3 participants