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 support for env variables with SFTP client. #1445

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

di-ov
Copy link

@di-ov di-ov commented Mar 4, 2025

This relates to and closes this issue: #1433

I basically saw how this was implemented in the exec() method in client.js and did a similar implementation.

I seems that it might be better to add an additional parameter to the sftp() method, i.e. in client.js it would appear as sftp(env, cb), I wasn't entirely sure, what do you think? Thanks!

@angelparaskov
Copy link

@mscdex we also would like to have such a solution.
May we expedite this one please?
Thanks

@mscdex
Copy link
Owner

mscdex commented Mar 4, 2025

I seems that it might be better to add an additional parameter to the sftp() method, i.e. in client.js it would appear as sftp(env, cb)

That would be the preferred approach.

@di-ov
Copy link
Author

di-ov commented Mar 5, 2025

@mscdex thanks for the help, I did the changes. If it looks okay, I'll have a look at updating the docs.

Dimitar Markov and others added 2 commits March 25, 2025 11:34
@di-ov di-ov requested a review from mscdex March 25, 2025 09:40
@mscdex
Copy link
Owner

mscdex commented Mar 26, 2025

All that's needed now is a unit test. You'll probably have to use one similar to the last one in the SFTP test script to verify the environment is being sent properly before starting the subsystem.

@di-ov di-ov force-pushed the add-sftp-env-option branch from 9db57fe to 3659269 Compare March 28, 2025 10:02
Co-authored-by: mscdex <[email protected]>
@di-ov di-ov requested a review from mscdex March 31, 2025 06:33
@di-ov
Copy link
Author

di-ov commented Apr 1, 2025

@mscdex so this look okay right? Would you be able to include it in the next release?

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.

3 participants