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(integration/fivetran): Fivetran source ingestion integration #14

Closed
wants to merge 17 commits into from

Conversation

shubhamjagtap639
Copy link
Owner

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

Copy link

@siddiquebagwan-gslab siddiquebagwan-gslab left a comment

Choose a reason for hiding this comment

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

Could you please add a document like we do have in Power BI to mention the concept mapping

@@ -505,6 +506,7 @@ def get_long_description():
"nifi",
"vertica",
"mode",
"fivetran",

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Now we added one unit test case on fivetran source


class SnowflakeDestinationConfig(BaseSnowflakeConfig):
database: str = Field(
default=None, description="The fivetran connector log database."

Choose a reason for hiding this comment

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

Is there any default for the database?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No
Even for log_schema config there is no default hence removing default value

default=None,
description="The destination platform where fivetran connector log tables are dumped.",
)
snowflake_destination_config: Optional[SnowflakeDestinationConfig] = pydantic.Field(

Choose a reason for hiding this comment

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

let's rename it to destination_config

Copy link
Owner Author

Choose a reason for hiding this comment

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

But this config is specific to snowflake destination only.

Choose a reason for hiding this comment

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

type is sufficient to understand SnowflakeDestinationConfig, later we can change it to Union[SnowflakeDestinationConfig|BigQueryDestinationConfig], so let's rename it to destination_config

@shubhamjagtap639 shubhamjagtap639 changed the title feat(integration/airbyte): Fivetran source ingestion integration feat(integration/fivetran): Fivetran source ingestion integration Oct 5, 2023
default=None,
description="The destination platform where fivetran connector log tables are dumped.",
)
snowflake_destination_config: Optional[SnowflakeDestinationConfig] = pydantic.Field(

Choose a reason for hiding this comment

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

type is sufficient to understand SnowflakeDestinationConfig, later we can change it to Union[SnowflakeDestinationConfig|BigQueryDestinationConfig], so let's rename it to destination_config

@hsheth2
Copy link

hsheth2 commented Nov 1, 2023

@shubhamjagtap639 can we close this PR?

@shubhamjagtap639
Copy link
Owner Author

@shubhamjagtap639 can we close this PR?

Yes, I am closing this PR

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.

3 participants