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

[CopyFilesOverSSH] Migrated to ssh2-sftp-client library #13312

Conversation

sdobrodeev
Copy link

@sdobrodeev sdobrodeev commented Jul 17, 2020

Task name: CopyFilesOverSSH

Description:

Documentation changes required: (Y/N) N

Added unit tests: (Y/N) N

Attached related issue: (Y/N) Y

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected

defer.resolve(true);
}
})
if (await this.sftpClient.stat(path)) {
Copy link
Contributor

@alexander-smolyakov alexander-smolyakov Jul 17, 2020

Choose a reason for hiding this comment

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

What a point to use stat method here? I assume we can use the exists method from ssh2-sftp-client. So code be more clear:

const isPathExist: boolean = await this.sftpClient.exists(path);
defer.resolve(isPathExist);

@sdobrodeev sdobrodeev force-pushed the users/sdobrodeev/feature13048_CopyFilesOverSSH_sftp_migration branch from 9298ae8 to 07a0004 Compare July 21, 2020 06:46
@sdobrodeev sdobrodeev force-pushed the users/sdobrodeev/feature13048_CopyFilesOverSSH_sftp_migration branch 2 times, most recently from 9792663 to 4a70555 Compare July 22, 2020 07:16
@sdobrodeev sdobrodeev force-pushed the users/sdobrodeev/feature13048_CopyFilesOverSSH_sftp_migration branch from 4a70555 to 402ae4b Compare July 22, 2020 17:46
@anatolybolshakov anatolybolshakov self-requested a review July 22, 2020 19:46
Copy link
Contributor

@anatolybolshakov anatolybolshakov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@sdobrodeev sdobrodeev requested a review from a team July 23, 2020 09:53
*/
private unixyPath(filePath) {
if (process.platform === 'win32') {
return filePath.replace(/\\/g, '/');
Copy link
Member

Choose a reason for hiding this comment

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

Does it works fine with full paths like 'C:\test\etc' which can contain ":"?

@sdobrodeev sdobrodeev requested a review from damccorm July 27, 2020 08:39
@sdobrodeev sdobrodeev force-pushed the users/sdobrodeev/feature13048_CopyFilesOverSSH_sftp_migration branch from 402ae4b to ed8ee28 Compare July 28, 2020 13:02
Copy link
Contributor

@alexander-smolyakov alexander-smolyakov left a comment

Choose a reason for hiding this comment

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

LGTM thanks, but also please take a look at the comment.

@@ -17,8 +17,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 0,
"Minor": 172,
"Patch": 3
"Minor": 173,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please bump up task version.

@sdobrodeev sdobrodeev force-pushed the users/sdobrodeev/feature13048_CopyFilesOverSSH_sftp_migration branch from ed8ee28 to 05c87c6 Compare July 31, 2020 08:23
@sdobrodeev sdobrodeev merged commit 8726073 into master Aug 3, 2020
@sdobrodeev sdobrodeev deleted the users/sdobrodeev/feature13048_CopyFilesOverSSH_sftp_migration branch August 3, 2020 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants