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

[2/n][dagster-airbyte] Scaffold DagsterAirbyteTranslator for rework #26205

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

maximearmstrong
Copy link
Contributor

@maximearmstrong maximearmstrong commented Nov 28, 2024

Summary & Motivation

Builds out a very barebones translator and props classes for the new version of the Airbyte cloud integration.

How I Tested These Changes

Tests will be added in subsequent PRs.

Comment on lines 9 to 10
class AirbyteConnectionTableProps(NamedTuple):
...
Copy link

Choose a reason for hiding this comment

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

NamedTuple requires at least one field to be defined. If this is intended as a placeholder for future development, use pass instead of ... to make the placeholder explicit.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@maximearmstrong maximearmstrong force-pushed the maxime/rework-airbyte-cloud-1 branch from 64d2d52 to 93a7844 Compare December 2, 2024 20:53
@maximearmstrong maximearmstrong force-pushed the maxime/rework-airbyte-cloud-2 branch from b1a69b1 to 0d85429 Compare December 2, 2024 20:53
@maximearmstrong maximearmstrong self-assigned this Dec 2, 2024
@maximearmstrong maximearmstrong marked this pull request as ready for review December 2, 2024 20:55
from dagster._serdes.serdes import whitelist_for_serdes


class AirbyteConnectionTableProps(NamedTuple): ...
Copy link

Choose a reason for hiding this comment

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

AirbyteConnectionTableProps is currently defined as an empty NamedTuple. Since this class is used as a parameter type in get_asset_spec(), it needs to have its fields defined to be instantiated correctly at runtime. Consider adding the required fields to the NamedTuple definition.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@maximearmstrong maximearmstrong changed the title [dagster-airbyte] Scaffold DagsterAirbyteTranslator for rework [2/n][dagster-airbyte] Scaffold DagsterAirbyteTranslator for rework Dec 2, 2024
Copy link
Contributor

dpeng817 commented Dec 3, 2024

I'm assuming the upstack is meant to be non-draft as well?

@maximearmstrong maximearmstrong force-pushed the maxime/rework-airbyte-cloud-1 branch from 93a7844 to 215d8cf Compare December 3, 2024 19:34
@maximearmstrong maximearmstrong force-pushed the maxime/rework-airbyte-cloud-2 branch from 2dc0534 to 9c0212c Compare December 3, 2024 19:34
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.

I think this needs to wait on approval on downstreams since this PR doesn't have full context. Removing from my queue for now.

Base automatically changed from maxime/rework-airbyte-cloud-1 to master December 5, 2024 20:52
@maximearmstrong maximearmstrong force-pushed the maxime/rework-airbyte-cloud-2 branch from 8397e6a to 1df12dd Compare December 5, 2024 20:53
@maximearmstrong maximearmstrong merged commit 5595598 into master Dec 5, 2024
1 check failed
@maximearmstrong maximearmstrong deleted the maxime/rework-airbyte-cloud-2 branch December 5, 2024 21:16
pskinnerthyme pushed a commit to pskinnerthyme/dagster that referenced this pull request Dec 16, 2024
…agster-io#26205)

## Summary & Motivation

Builds out a very barebones translator and props classes for the new
version of the Airbyte cloud integration.

## How I Tested These Changes

Tests will be added in subsequent PRs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants