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.

Sign up for GitHub

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

Jump to bottom

[1/n][dagster-fivetran] Scaffold FivetranWorkspace for rework #25750

Merged
merged 4 commits into from
Nov 8, 2024
Merged

[1/n][dagster-fivetran] Scaffold FivetranWorkspace for rework #25750

merged 4 commits into from
Nov 8, 2024
+145 −1

Conversation

maximearmstrong
Copy link
Contributor

@maximearmstrong maximearmstrong commented Nov 5, 2024 •
edited
Loading

Summary & Motivation

Builds out a very barebones resource and client classes for the new version of the Fivetran integration.

How I Tested These Changes

Tests will be added in subsequent PRs.

@maximearmstrong Graphite App
Copy link
Contributor Author

maximearmstrong commented Nov 5, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@maximearmstrong maximearmstrong changed the title [dagster-fivetran] Add FivetranWorkspace foundation for rework [dagster-fivetran] Scaffold FivetranWorkspace for rework Nov 5, 2024
@maximearmstrong maximearmstrong mentioned this pull request Nov 5, 2024
Merged
@maximearmstrong maximearmstrong marked this pull request as ready for review November 5, 2024 18:40
@maximearmstrong maximearmstrong self-assigned this Nov 5, 2024
dpeng817
dpeng817 reviewed Nov 5, 2024
View reviewed changes
python_modules/libraries/dagster-fivetran/dagster_fivetran/v2/resources.py Outdated
return {"content_type": self.content_type.value, "properties": self.properties}

@classmethod
def from_cached_data(cls, data: Mapping[Any, Any]) -> "FivetranContentData":
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this one necessary? Feel like this class should just be @whitelist_for_serdes if we need to cache.

Copy link
Contributor Author

@maximearmstrong maximearmstrong Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback!

This was the original pattern for Power BI, but I see that it was updated to use @whitelist_for_serdes since then.

I based my pattern here on the Tableau integration, which was not updated to use @whitelist_for_serdes yet.

Changes for this PR made in 4dfe34d, I will also update Tableau in another PR.

dpeng817
dpeng817 previously requested changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@dpeng817 dpeng817 left a comment

Choose a reason for hiding this comment

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

Comment, otherwise lgtm

benpankow
benpankow reviewed Nov 5, 2024
View reviewed changes
python_modules/libraries/dagster-fivetran/dagster_fivetran/v2/resources.py Outdated

_client: FivetranClient = PrivateAttr(default=None)

def get_client(self) -> FivetranClient:
Copy link
Member

Choose a reason for hiding this comment

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

Curious what the motivation for separating out the client is here - just to separate out the workspace loading and the actual API interactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly - this is the current pattern for Tableau and Looker.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me 👍

@maximearmstrong maximearmstrong mentioned this pull request Nov 5, 2024
Merged
@maximearmstrong maximearmstrong changed the title [dagster-fivetran] Scaffold FivetranWorkspace for rework [1/n][dagster-fivetran] Scaffold FivetranWorkspace for rework Nov 7, 2024
@maximearmstrong maximearmstrong mentioned this pull request Nov 7, 2024
Merged
dpeng817
dpeng817 approved these changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@dpeng817 dpeng817 left a comment

Choose a reason for hiding this comment

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

nice!

@maximearmstrong maximearmstrong mentioned this pull request Nov 7, 2024
Merged
This was referenced Nov 8, 2024
Merged
Merged
@maximearmstrong maximearmstrong merged commit 0a0c77b into master Nov 8, 2024
1 check passed
@maximearmstrong maximearmstrong deleted the maxime/rework-fivetran-1 branch November 8, 2024 17:00
This was referenced Nov 12, 2024
Merged
Open
Open
Draft
Draft
This was referenced Nov 20, 2024
Draft
Draft
Open
Open
Open
Open
Draft
Draft
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpeng817 dpeng817 dpeng817 approved these changes

@OwenKephart OwenKephart Awaiting requested review from OwenKephart

@benpankow benpankow Awaiting requested review from benpankow

Assignees

@maximearmstrong maximearmstrong

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maximearmstrong @benpankow @dpeng817