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

🎉 New Destination: Propel #35427

Conversation

margaritagomez
Copy link

@margaritagomez margaritagomez commented Feb 19, 2024

What

This PR introduces a new Propel destination. Resolves #35417.

How

The new destination was built in Go, in a separate private repository. The public docker image can be found here.

We have built our own unit and integration tests to assert its proper behavior, as well as tested it by running Airbyte locally. Let me know if the repo should be public for reviews.

Recommended reading order

  1. propel.md
  2. metadata.yaml
  3. oss_registry.json

🚨 User Impact 🚨

This introduces our first version of the destination so it is marked as 0.0.1.

Unit tests

Our unit tests mock our Propel API and focus on asserting errors, logs and successful writes.
image

Integration tests

These are run every time a pull request is created for our destination repo, where the docker is built, and the three spec, check and write commands are run. We then poll to our e2e Propel account to make sure all records were correctly inserted.
image

Airbyte tests

Here are a few connectors we set up that successfully moved data to Propel. Each Data Pool corresponds to an Airbyte stream.
image

Pre-merge Actions

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Connector version is set to 0.0.1
    • Dockerfile has version 0.0.1
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples --> This link does not work
    • docs/integrations/<source or destination>/<name>.md including changelog with an entry for the initial version. See changelog example
    • docs/integrations/README.md

Copy link

vercel bot commented Feb 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 9:06pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation community labels Feb 19, 2024
Copy link
Contributor

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@CLAassistant
Copy link

CLAassistant commented Feb 19, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Margarita Gomez seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

@acossta acossta left a comment

Choose a reason for hiding this comment

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

Left some comments

docs/integrations/destinations/propel.md Outdated Show resolved Hide resolved
docs/integrations/destinations/propel.md Outdated Show resolved Hide resolved
@margaritagomez margaritagomez marked this pull request as ready for review February 20, 2024 00:32
@margaritagomez margaritagomez requested a review from a team as a code owner February 20, 2024 00:32
docs/integrations/destinations/propel.md Outdated Show resolved Hide resolved
Copy link
Contributor

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

Hey @margaritagomez and @acossta! Thank you both for the contribution. It might take us a second, let me figure out who's the right person to review and figure out the pieces for how to get this in.

A few concerns that we need to work through:

  • I think we have connectors that we only have as an image (and not code in our repo), but I'm not certain how we go about them.
  • We might require some security code audit and docker image audit to be wired up — part of this is on our side.

Do you have specific use cases or folks who would like to beta-test the destination?

releaseStage: alpha
documentationUrl: https://docs.airbyte.com/integrations/destinations/propel
tags:
- language:unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. IS this because the tag is required, but the value is irrelevant?

Copy link
Author

Choose a reason for hiding this comment

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

I just used the other third party connectors as a base. Since those are in Javascript and the language was set as unknown, I set it as such too. Do let me know if setting it as go or golang would be appropriate.

documentationUrl: https://docs.airbyte.com/integrations/destinations/propel
tags:
- language:unknown
ab_internal:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if these make sense — and I'm not sure why we default them to 100.

Copy link
Author

Choose a reason for hiding this comment

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

Same as above, the others had it as 100 😕 but let me know if those should change

@margaritagomez
Copy link
Author

margaritagomez commented Feb 21, 2024

@natikgadzhi Hi, thanks for the comments!

We have customers who wish to migrate their data from different sources (SaaS apps and databases) to Propel. We would also like to use this Airbyte destination as leverage for a fast integration to our product.

The destination repo is private as of now, but we can make it public if that helps the audits.

@marcosmarxm
Copy link
Member

The contribution is great, @margaritagomez! I'm concerned about adding an external connector to the main catalog without available source code. This implies it's accepted as an Airbyte connector, but without access to the code, security issues can't be addressed or audited.

There are some work being done to have a better way to share 3rd party connectors in the future.

If your customers want to get data using Airbyte they can add the connector manually as custom one.

If the connector is in the main catalog people will start opening issues in the airbyte repo and we don't have access or ways to resolve them.

In my opinion Propel should follow the same as PlanetScale is doing for now: https://planetscale.com/docs/integrations/airbyte

@margaritagomez
Copy link
Author

@marcosmarxm I understand the concern! We are allowing customers follow the same path as you say with PlanetScale.

Is there a way to move ahead with this PR by making our destination repository public, like folks at Faros AI in their Airbyte connectors repo, and have users file issues over there? Here is our public repo just in case.

@natikgadzhi
Copy link
Contributor

@margaritagomez to merge the RP and make a new 3rd party destination available in Cloud, we'd need to do some extra work:

  • get a sandbox/test account into our secrets manager
  • either at the very least do a thorough one-time test, and setup CATs (acceptance tests)
  • do a code / security audit of the destination code.

Admittedly, we haven't prioritized reviewing 3rd party destinations recently, so we're not entirely quick with these steps.

In the meantime, I'm trying to check if adding a public Docker image as a custom connector works on Cloud (the form is there, but I think I found a bug and it hangs) — I'll report back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation community new-connector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New connector: Propel destination
6 participants