🎉 New SFTP source connector #13120
🎉 New SFTP source connector #13120
Conversation
|
/test connector=connectors/source-sftp
|
github-actions
bot
added
area/connectors
|
|
|
|
||
|
|
||
| @Override | ||
| public AirbyteCatalog discover(JsonNode config) throws Exception { |
There was a problem hiding this comment.
Could you add a comment about what the implementation of this method does?
There was a problem hiding this comment.
It is already documented in the parent interface Source
/**
* Discover the current schema in the source.
*
* @param config - integration-specific configuration object as json. e.g. { "username": "airbyte",
* "password": "super secure" }
* @return Description of the schema.
* @throws Exception - any exception.
*/
AirbyteCatalog discover(JsonNode config) throws Exception;
But maybe it may sense to add an extra one for Sftp implementation with more sftp discover details
...ons/connectors/source-sftp/src/main/java/io/airbyte/integrations/source/sftp/SftpSource.java
Outdated
Show resolved
Hide resolved
...ns/connectors/source-sftp/src/main/java/io/airbyte/integrations/source/sftp/SftpCommand.java
Show resolved
Hide resolved
|
/test connector=connectors/source-sftp
Build PassedTest summary info: |
| package io.airbyte.integrations.source.sftp; | ||
|
|
||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.jcraft.jsch.*; |
There was a problem hiding this comment.
please, uncollapse the imports
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.io.ByteArrayInputStream; | ||
| import java.util.*; |
There was a problem hiding this comment.
please, uncollapse the imports
| /** | ||
| * Adapter from JSch's logger interface to our log4j | ||
| */ | ||
| private static class LogAdapter implements com.jcraft.jsch.Logger { |
There was a problem hiding this comment.
probably it makes sense to move this class into a separate file?
There was a problem hiding this comment.
It was pushed mistakenly. I have to remove it anyway
| public class SftpFileParserFactory { | ||
|
|
||
| public static SftpFileParser createInstance(SupportedFileExtension fileExtension) { | ||
| switch (fileExtension) { |
There was a problem hiding this comment.
can we somehow re-use the existing instance of the corresponding parser? probably make it singletone?
| "type": "string", | ||
| "order": 0 | ||
| }, | ||
| "host_address": { |
There was a problem hiding this comment.
please use host instead, to make it more generic
| "required": ["user_name", "host_address", "port"], | ||
| "additionalProperties": true, | ||
| "properties": { | ||
| "user_name": { |
There was a problem hiding this comment.
please use username instead, to make it more generic
| import io.airbyte.commons.resources.MoreResources; | ||
| import io.airbyte.integrations.standardtest.source.SourceAcceptanceTest; | ||
| import io.airbyte.integrations.standardtest.source.TestDestinationEnv; | ||
| import io.airbyte.protocol.models.*; |
There was a problem hiding this comment.
please, uncollapse the imports
| import java.io.InputStream; | ||
| import java.util.List; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; |
There was a problem hiding this comment.
please, uncollapse the imports
| import java.io.InputStream; | ||
| import java.util.List; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; |
There was a problem hiding this comment.
please, uncollapse the imports
|
/test connector=connectors/source-sftp
Build PassedTest summary info: |
|
@alexandr-shegeda @grishick, I finished with updates. Could you please have a look once more? |
|
|
||
| private void checkIfConnected() { | ||
| if (!client.isConnected()) { | ||
| LOGGER.info("SFTP client is not connected."); |
There was a problem hiding this comment.
nit: this log message should also mention that it's about to connect. E.g. "SFTP client is not connected. Attempting to reconnect."
...ns/connectors/source-sftp/src/main/java/io/airbyte/integrations/source/sftp/SftpCommand.java
Show resolved
Hide resolved
| }, | ||
| "file_type": { | ||
| "title": "File type", | ||
| "description": "The file type for data sync", |
There was a problem hiding this comment.
If this parameter allows multiple coma separated values, then the name should be "File types" and description should be "Coma separated file types. Currently only 'csv' and 'json' types are supported.".
There was a problem hiding this comment.
Yes, sure. I guess we need to request multi select drop down from UI team to avoid this comma separated thing it's not user friendly
| * @throws Exception - any exception. | ||
| */ | ||
| @Override | ||
| public AirbyteConnectionStatus check(JsonNode config) throws Exception { |
There was a problem hiding this comment.
From what I can see, this method actually can never throw Exception, because all code that can throw an exception is inside try-catch block.
There was a problem hiding this comment.
Thanks for noticing. I'll remove it
|
Hi @grishick, thanks for the review! I resolved all points which you mentioned |
|
/test connector=connectors/source-sftp
Build PassedTest summary info: |
|
/publish connector=connectors/source-sftp
|
|
/publish connector=connectors/source-sftp
|
|
/publish connector=connectors/source-sftp
|
* 11797 New SFTP source * 11797 Bump sftp source version * auto-bump connector version Co-authored-by: Octavia Squidington III <[email protected]>
What
How
Add SFTP Connector using Java
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/SUMMARY.mddocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampledocs/integrations/README.mdairbyte-integrations/builds.mdAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing/publishcommand described hereTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.