-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
🎉 Source Cockroachdb: Added new source for Cockroachdb #4382
Conversation
…' into etsybaev/1705-cockroachdb-source
...-cockroachdb/src/main/java/io/airbyte/integrations/source/cockroachdb/CockroachdbSource.java
Outdated
Show resolved
Hide resolved
...tegration/java/io/airbyte/integrations/source/cockroachdb/CockroachSourceAcceptanceTest.java
Outdated
Show resolved
Hide resolved
...kroachdb/src/test/java/io/airbyte/integrations/source/cockroachdb/CockroachdbSourceTest.java
Outdated
Show resolved
Hide resolved
/test connector=connectors/source-cockroachdb
|
@@ -786,7 +787,7 @@ private ConfiguredAirbyteCatalog getConfiguredCatalogWithOneStream(final String | |||
return catalog; | |||
} | |||
|
|||
private AirbyteCatalog getCatalog(final String defaultNamespace) { | |||
protected AirbyteCatalog getCatalog(final String defaultNamespace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these changed to protected? we should keep private unless required to change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to override it in CockroachDbJdbcSourceAcceptanceTest. Basic Catalog contains a case with the table without PK meanwhile the Cockroach DB is not supposed to have a tables without PK. If you do not set it explicitly - it will anyway automatically add additionally.
| `timestamp without timezone` | string | may be written as a native date type depending on the destination | | ||
| `uuid` | string | | | ||
|
||
**Note:** arrays for all the above types as well as custom types are supported, although they may be de-nested depending on the destination. Byte arrays are currently unsupported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens to byte arrays? does the connector just break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I copy-pasted the template from the postgres source and probably missed to clean this Note. Byte arrays also works, added additional test for it to comprehensive tests. Thanks
airbyte-integrations/connectors/source-cockroachdb/build.gradle
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- can you make sure to update all the docs mentioned in the checklist? some of them are not updated
airbyte-config/init/src/main/resources/seed/source_definitions.yaml
Outdated
Show resolved
Hide resolved
/test connector=connectors/source-cockroachdb
|
/test connector=connectors/source-cockroachdb
|
# Conflicts: # docs/integrations/README.md
/test connector=connectors/source-cockroachdb
|
/publish connector=connectors/source-cockroachdb
|
/publish connector=connectors/source-cockroachdb
|
/test connector=connectors/source-cockroachdb
|
/publish connector=connectors/source-cockroachdb
|
What
Added CockroachDB source connector.
How
Added CockroachDB source connector.
Some of data type returns wrong values, but those issues are same for all of DB. A follow-up ticket's been created #4408
Pre-merge Checklist
Expand the checklist which is relevant for this PR.
Connector checklist
airbyte_secret
in the connector's spec./gradlew :airbyte-integrations:connectors:<name>:integrationTest
./test connector=connectors/<name>
command as documented here is passing.README.md
docs/SUMMARY.md
if it's a new connectordocs/integrations/<source or destination>/<name>
.docs/integrations/...
. See changelog exampledocs/integrations/README.md
contains a reference to the new connector/publish
command described hereConnector Generator checklist
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changes