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

[3/n][dagster-fivetran] Implement FivetranClient for rework #25756

Merged
merged 9 commits into from
Nov 8, 2024
Merged

[3/n][dagster-fivetran] Implement FivetranClient for rework #25756

merged 9 commits into from
Nov 8, 2024

Conversation

maximearmstrong
Copy link
Contributor

@maximearmstrong maximearmstrong commented Nov 5, 2024 •
edited
Loading

Summary & Motivation

This PR implements FivetranClient, which is based on the legacy FivetranResource code. Basically reusing how make_request was implemented and adding more methods to leverage in a subsequent PR to fetch the Fivetran workspace data.

How I Tested These Changes

Additional unit test

@maximearmstrong maximearmstrong mentioned this pull request Nov 5, 2024
Merged
@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 mentioned this pull request Nov 5, 2024
Merged
@maximearmstrong maximearmstrong self-assigned this Nov 5, 2024
@maximearmstrong maximearmstrong marked this pull request as ready for review November 5, 2024 23:42
@maximearmstrong maximearmstrong force-pushed the maxime/rework-fivetran-3 branch 2 times, most recently from 02ccc8d to 3cedb5e Compare November 6, 2024 22:06
benpankow
benpankow approved these changes Nov 6, 2024
View reviewed changes
Copy link
Member

@benpankow benpankow left a comment

Choose a reason for hiding this comment

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

Nice, looks good to me

@maximearmstrong maximearmstrong changed the title [dagster-fivetran] Implement FivetranClient for rework [3/n][dagster-fivetran] Implement FivetranClient for rework Nov 7, 2024
@maximearmstrong maximearmstrong mentioned this pull request Nov 7, 2024
Merged
@maximearmstrong maximearmstrong mentioned this pull request Nov 7, 2024
Merged
dpeng817
dpeng817 requested changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@dpeng817 dpeng817 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I won't block review on this, but I'd like to get your thoughts regarding testing strategy here.

One thing that we also started to do with dlift + airlift was have some test battery running live against the integration. IMO when a new person onboards to the integration, something like that is pretty invaluable in terms of getting started with the tooling, I will always trust it better than just pure unit tests especially when running against an external system which can change at any point. The _lack_of these I think is a major reason why we don't feel confidence in integrations which haven't been touched in a while. Doesn't have to be super complicated, but having a "kitchen sink" testbed for the integration similar to what we did for dlift/airlift: https://github.com/dagster-io/dagster/blob/master/examples/experimental/dagster-dlift/kitchen-sink/Makefile could be very nice. Then, you can leverage dagster-buildkite to make it only run in the nightly + on PRs that touch integration code.

Thoughts?

python_modules/libraries/dagster-fivetran/dagster_fivetran_tests/experimental/conftest.py Outdated
@pytest.fixture(
name="workspace_data_api_mocks_fn",
)
def workspace_data_api_mocks_fn_fixture(
Copy link
Contributor

Choose a reason for hiding this comment

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

In general I find API mocks like this to be relatively inflexible and hard to parse. How would you feel about instead implementing a "client fake" similar to how we did for airlift and dlift? Something like this:
https://github.com/dagster-io/dagster/blob/master/examples/experimental/dagster-dlift/dagster_dlift/test/client_fake.py

I've found it to be more composable and human-readable than mocking. I won't necessarily block review on this, but every time I've based a testing strategy around mocks I've regretted it. I assume this is also somewhat inherited from the old testing strategy, but I think if we're spending time in this code we might as well improve the situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment in next PR

python_modules/libraries/dagster-fivetran/dagster_fivetran_tests/experimental/conftest.py
},
}

SAMPLE_CONNECTOR_DETAILS = {
Copy link
Contributor

Choose a reason for hiding this comment

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

comment on how these were generated? What is our plan if the schema changes, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in e4892e8

dpeng817
dpeng817 approved these changes Nov 8, 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.

Oops meant to approve.

This was referenced Nov 8, 2024
Merged
Merged
Base automatically changed from maxime/rework-fivetran-2 to master November 8, 2024 17:40
@maximearmstrong
Copy link
Contributor Author

One thing that we also started to do with dlift + airlift was have some test battery running live against the integration. IMO when a new person onboards to the integration, something like that is pretty invaluable in terms of getting started with the tooling, I will always trust it better than just pure unit tests especially when running against an external system which can change at any point. The _lack_of these I think is a major reason why we don't feel confidence in integrations which haven't been touched in a while. Doesn't have to be super complicated, but having a "kitchen sink" testbed for the integration similar to what we did for dlift/airlift: https://github.com/dagster-io/dagster/blob/master/examples/experimental/dagster-dlift/kitchen-sink/Makefile could be very nice. Then, you can leverage dagster-buildkite to make it only run in the nightly + on PRs that touch integration code.

Thoughts?

I'm very much in favor of this strategy. I agree that unit tests mocking external services are not enough to make sure that the integration works properly. I'll open a ticket for that.

@maximearmstrong maximearmstrong merged commit 83582b5 into master Nov 8, 2024
1 check passed
@maximearmstrong maximearmstrong deleted the maxime/rework-fivetran-3 branch November 8, 2024 20:22
This was referenced Nov 12, 2024
Merged
Merged
Open
Open
Open
This was referenced Nov 20, 2024
Open
Open
Merged
Merged
Merged
Merged
Draft
Draft
Open
@maximearmstrong maximearmstrong mentioned this pull request Nov 27, 2024
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

@benpankow benpankow benpankow approved these changes

@OwenKephart OwenKephart Awaiting requested review from OwenKephart

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