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/prefect): DataHub Prefect Emitter #1

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

siddiquebagwan
Copy link

No description provided.

@asikowitz asikowitz self-requested a review June 22, 2023 16:03
Copy link

@asikowitz asikowitz left a comment

Choose a reason for hiding this comment

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

Haven't had the chance to go through this yet, but briefly scanning it looks like a lot of files are boilerplate because this is in a separate repo. If we are going to make this plugin public, can we include it in the OSS repo instead? In general I prefer monorepos, we have our airflow plugin in there already, and it means we don't have to duplicate some files / logic.

@siddiquebagwan
Copy link
Author

siddiquebagwan commented Jun 28, 2023

Haven't had the chance to go through this yet, but briefly scanning it looks like a lot of files are boilerplate because this is in a separate repo. If we are going to make this plugin public, can we include it in the OSS repo instead? In general I prefer monorepos, we have our airflow plugin in there already, and it means we don't have to duplicate some files / logic.

It is in a separate repo as per the integration steps of Prefect, each plugin has a repo, please refer to their site https://docs.prefect.io/2.10.17/integrations/ Here they have listed all of their existing plugins/blocks, Our plugin will appear here
Aobove folder structure is auto-generated by their tool: https://github.com/PrefectHQ/prefect-collection-template#quickstart

@asikowitz
Copy link

So I see that all the listed integrations are in their own repo and I see the template they provide, but I don't see anywhere in the docs that either of these are requirements. https://docs.prefect.io/2.10.18/integrations/contribute/#generate-a-project just states "To help you get started with your integration, we've created a template that gives the tools you need to create and publish your integration", but it provides some tools that we already have set up, like linters, code coverage, github workflows, and versioning. We also need to figure out how we want to handle docs for this connector -- are we just going to link the prefect docs on https://datahubproject.io/docs/ or do we want to put them on that site; if so, do we use our styling or Prefect's? It'd also be very nice to integrate the release process of this connector with our existing release process, than have it separate.

At the least, I think we can take pretty much everything in this repo and put it somewhere in the monorepo instead. The main piece to work out would be the release process -- versioning and github workflows.

@hsheth2
Copy link

hsheth2 commented Jul 7, 2023

Agree with Andrew here - I don't think there's any real blockers to putting this inside the main datahub monorepo, and doing so will make linting/docs/testing much easier.

It should either go in the main metadata-ingestion directory, or if it needs to be a separate package, it can go in metadata-ingestion-modules

@asikowitz
Copy link

Also, can you provide test instance credentials / the local prefect setup you used to do testing? Not in the code, just here or in slack somewhere, thanks!

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.

5 participants