Skip to content

Add an option to place the temporary path for ControlDirectory in a user-set location. #16

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

Merged
merged 5 commits into from
Sep 4, 2020

Conversation

lwb4
Copy link
Contributor

@lwb4 lwb4 commented Sep 1, 2020

I'd like to be able to put the temporary directory where the control socket is created is a custom location.

This patch enables that by adding the .control_directory() function to the SessionBuilder.

Tried to match code style best I could, but please give suggestions for naming/style consistency as appropriate.

I tested manually, with and without the control_directory option. Didn't add anything to tests/openssh.rs as I didn't see anything similar for keyfile, which was my guide for this change.

If tests are requested, please give some ideas about how this should best be done -- relatively new to Rust so I may need a push in the right direction :)

@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #16 into master will decrease coverage by 0.04%.
The diff coverage is 71.42%.

Impacted Files Coverage Δ
src/builder.rs 86.88% <71.42%> (-0.19%) ⬇️

@jonhoo jonhoo mentioned this pull request Sep 3, 2020
@jonhoo
Copy link
Collaborator

jonhoo commented Sep 3, 2020

This looks great, thank you! It'd also fix #17 🎉
I left some minor notes.

I'd like to see a test, which I think should be pretty easy. Just copy the it_connects test, modify it to set control_dir, and check that there is indeed a control file in the path given to control_dir.

@lwb4
Copy link
Contributor Author

lwb4 commented Sep 3, 2020

@jonhoo Thanks! Think I addressed your comments, let me know if that covers it.

@jonhoo
Copy link
Collaborator

jonhoo commented Sep 3, 2020

Thanks! Let me take a look

tests/openssh.rs Outdated
#[tokio::test]
#[cfg_attr(not(ci), ignore)]
async fn control_dir() {
let session = SessionBuilder::default().control_directory(&path()).connect(&addr()).await.unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you need to run rustfmt on this file — that's what CI is complaining about :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it's related to my changes -- I'm seeing lots of this error in that file:

error[E0670]: `async fn` is not permitted in the 2015 edition
  --> /home/lincolnb/rust/openssh-rs/tests/openssh.rs:14:1
   |
14 | async fn it_connects() {
   | ^^^^^ to use `async fn`, switch to Rust 2018
   |
   = help: set `edition = "2018"` in `Cargo.toml`
   = note: for more on editions, read https://doc.rust-lang.org/edition-guide

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's weird... Do you have a very old version of Rust installed/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, turns out you have to specify --edition 2018 on the command line!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohhh, for rustfmt. Yeah, you're running into rust-lang/rustfmt#4074, which is awkward. You can also add a file in the current directory called rustfmt.toml that contains edition = "2018". It's.... not great.

@jonhoo
Copy link
Collaborator

jonhoo commented Sep 3, 2020

You may also want to merge with master to get the CI fixes I just pushed :)

@jonhoo jonhoo merged commit 9633d70 into openssh-rust:master Sep 4, 2020
jonhoo added a commit that referenced this pull request Sep 4, 2020
@jonhoo
Copy link
Collaborator

jonhoo commented Sep 4, 2020

Excellent, thanks for sticking with it! Published in 0.6.3 🎉

@lwb4
Copy link
Contributor Author

lwb4 commented Sep 5, 2020

Thanks for merging!

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

Successfully merging this pull request may close these issues.

3 participants