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 ssh.process not setting ssh_process.cwd #2241

Merged
merged 2 commits into from
Jul 29, 2023

Conversation

peace-maker
Copy link
Member

The cwd wasn't passed to the ssh_process created in ssh.process, so the ssh_process.cwd attribute was never set. The process is running in the correct working directory though, but the cwd attribute didn't reflect it.

This passes the cwd along now.

s = ssh(..)
io = s.process("pwd", cwd="/tmp")
io.cwd # was ".", is now "/tmp"
io.recvall() # b"/tmp\n"

Fixes #1479

The cwd wasn't passed to the ssh_process created in ssh.process,
so the ssh_process.cwd attribute was never set.

This passes the cwd along now.
```
s = ssh(..)
io = s.process("pwd", cwd="/tmp")
io.cwd # "."
io.recvall() # b"/tmp\n"
```
@peace-maker peace-maker added this to the 4.12.0 milestone Jul 28, 2023
@Arusekk
Copy link
Member

Arusekk commented Jul 29, 2023

I'm not sure what we should do with wd= vs cwd= in general, and should this be considered an API change (because no matter how bugfixy it is, we cannot push it to stable, because it would kill our semantic versioning).

@Arusekk Arusekk modified the milestones: 4.12.0, 4.10.1 Jul 29, 2023
@peace-maker
Copy link
Member Author

Depends on if we consider ssh_channel public API. I don't know how you would use it directly without going though ssh.

@Arusekk
Copy link
Member

Arusekk commented Jul 29, 2023

Okay, but I think we should clean up this process(cwd=...) vs system(wd=...) thing on dev branch while at it.

@Arusekk Arusekk merged commit a0c9da0 into Gallopsled:stable Jul 29, 2023
@peace-maker peace-maker deleted the ssh_process_cwd branch July 30, 2023 07:40
@peace-maker
Copy link
Member Author

Yes, but that would be a breaking change. 😄 Are we fine with those in minor versions?

@Arusekk
Copy link
Member

Arusekk commented Jul 30, 2023 via email

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.

2 participants