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

feat: add remoteFilePath support and create remote directory in sftp #437

Merged
merged 4 commits into from
Apr 30, 2024

Conversation

Gauravudia
Copy link
Contributor

@Gauravudia Gauravudia commented Apr 22, 2024

Description

  • Added support for remote file paths in function parameter and removed support for remote directory parameter.
  • The remote file path parameter provides more flexibility. Since it will be an input in the UI, we can directly pass it to the Upload function, whereas with the remote directory, we had to explicitly build the path.
  • Create a remote directory if it does not exist.

Linear Ticket

Resolved INT-2048
https://linear.app/rudderstack/issue/INT-2048/go-kit-create-remote-directory-in-sftp

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@Gauravudia Gauravudia self-assigned this Apr 22, 2024
@Gauravudia Gauravudia marked this pull request as ready for review April 22, 2024 05:07
@koladilip
Copy link
Contributor

should we also automatically create folder when file is uploaded?

@Gauravudia
Copy link
Contributor Author

should we also automatically create folder when file is uploaded?

Didn't get this. We are creating the remote directory/folder when file is uploaded

@Gauravudia Gauravudia changed the title feat: create remote directory in sftp feat: add remoteFilePath support and create remote directory in sftp Apr 28, 2024
@Gauravudia Gauravudia enabled auto-merge (squash) April 30, 2024 05:35
@Gauravudia Gauravudia requested a review from cisse21 April 30, 2024 05:36
@@ -138,10 +138,11 @@ func TestUpload(t *testing.T) {

mockSFTPClient := mock_sftp.NewMockClient(ctrl)
mockSFTPClient.EXPECT().Create(gomock.Any()).Return(&nopWriteCloser{remoteBuf}, nil)
mockSFTPClient.EXPECT().MkdirAll(gomock.Any()).Return(nil)
Copy link
Contributor

@mihir20 mihir20 Apr 30, 2024

Choose a reason for hiding this comment

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

would it be better if instead of using gomock.Any() actual value that is expected for the fn is used? i.e someRemotePath
Similarly for other mocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make any difference? Used Any() to increase the readability

Copy link
Contributor

Choose a reason for hiding this comment

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

Using actual expected value, is better for testing point of view.

@Gauravudia Gauravudia merged commit cd85cf5 into main Apr 30, 2024
12 checks passed
@Gauravudia Gauravudia deleted the feat.sftp-remote-dir branch April 30, 2024 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants