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 CDK: Dotnet/.NET/C# implementation + free exchange rates source connector #7945

Merged
merged 13 commits into from
Dec 13, 2021
Merged

Conversation

mrhamburg
Copy link
Contributor

@mrhamburg mrhamburg commented Nov 13, 2021

What

Added 2 items:

  1. A dotnet based CDK for connector development, including unit tests and a fluent stream builder. Should be on feature parity with the python based CDK, if not, please let me know where there are any gaps
  2. A dotnet cdk implemented exchange rates api which 1. does not require an api key and 2. which allows for retrieval of all base symbols

While there are some open ends, see the list below, I already pushed this draft PR for early review by the internal Airbyte team. Fail fast 😉

How

Implemented features and their dependencies:

  • Airbyte protocol
    • Not sure if I implemented the primary key type correctly, had some issues when trying out the exchange rates implementation, resolved it, but did not verify the solution
  • HttpStream
    • Backoff and retry mechanism (using Polly)
    • Basic and Oauth authorization implementation (using Flurl)
    • Catalog schema validation (using JsonSchema.Net)
  • CommandLine implementation read and to some extent write, but I saw that the python CDK does not have any helper classes for creating write connectors yet (using CommandLineParser)
  • A fluent Http stream builder, personal preference, off course you can implement classes as well, if you prefer
  • Unit tests where needed
  • Implements the "_limit" and "_page_size" general configuration

Recommended reading order

  1. C# CDK -> airbyte/airbyte-cdk/dotnet
  2. C# CDK Implementation example -> airbyte/airbyte-integrations/connectors/source-exchange-rates-free

Note, this is a draft PR. the following items are still open

  • Integration with current build and release pipelines
  • The dotnet cdk has a dockerfile for testing and building a nuget package and pushing a new version of this nuget package to a nuget feed to host the CDK library. I doubt Airbyte already has a nuget feed, details for this feed will need be integrated with the CI/CD pipelines (done via ENV variables)
  • The placement of the gitignore for dotnet related items can be improved, currently all individual connectors will need to use their own gitignore to make sure build items are excluded from git
  • Documentation:
    • Dotnet CDK
    • Fluent stream builder
    • Free exchange rates source

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • 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.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/SUMMARY.md
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • 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.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the new connector version is published, connector version bumped in the seed directory as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Connector Generator

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed.

@CLAassistant
Copy link

CLAassistant commented Nov 13, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added area/connectors Connector related issues CDK Connector Development Kit labels Nov 13, 2021
@marcosmarxm
Copy link
Member

marcosmarxm commented Nov 18, 2021

Hey @mrhamburg this contribution is amazing! Would be possible to you point the parts you already know you need help improving?

@michel-tricot
Copy link
Contributor

whoa 🎉 , thank you @mrhamburg!

@sherifnada
Copy link
Contributor

@mrhamburg superb! Let us know how we can help

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Nov 24, 2021
@mrhamburg
Copy link
Contributor Author

@marcosmarxm, @sherifnada, @michel-tricot. I have added a connector generator, added some documentation, made a github actions pipeline (first time, so it still needs testing) and created a nuget account: NuGet

My questions for now are:

  • who(m) should I provide the api keys for nuget to push new versions of the cdk?

What I am still working on:

  • Github action for releasing a new version of the CDK will need to be tested (I might concede this part, never done this before)
  • Documentation, currently at 80%

@marcosmarxm
Copy link
Member

@sherifnada can you help here?

@marcosmarxm
Copy link
Member

marcosmarxm commented Dec 7, 2021

@mrhamburg what do you think in release this in your repo like Faros.ai is doing for the Typescript CDK? We can add your contribution inside our docs for people who want to build connectors using C#. This will helps you have a more concise code and workflow to improve the CDK.

@marcosmarxm marcosmarxm self-assigned this Dec 7, 2021
@mrhamburg
Copy link
Contributor Author

@marcosmarxm, will do, I will setup a separate repo and change this PR to documentation only.

@sherifnada
Copy link
Contributor

@mrhamburg apologies for the late response, to add some context to marcos' comment:

We discussed this a bit within the team. We would really love to maintain this, but we unfortunately don't yet have the skillset to maintain a production grade C# CDK (we are proficient in java/python/a little typescript). We're more than happy to provide a review and showcase this CDK in the docs etc.. and redirect to an external repository (and maybe in time, adopt the CDK and maintain it in-house).

Please LMK when you've created the external repo and I'd be happy to take a look

@github-actions github-actions bot removed area/connectors Connector related issues CDK Connector Development Kit labels Dec 9, 2021
@mrhamburg mrhamburg marked this pull request as ready for review December 9, 2021 22:06
@mrhamburg
Copy link
Contributor Author

@marcosmarxm, @sherifnada. I have changed this PR to documentation only and prepared a repo for the dotnet based cdk and its connectors: https://github.com/mrhamburg/airbyte.cdk.dotnet

@marcosmarxm
Copy link
Member

awesome work @mrhamburg !

@marcosmarxm marcosmarxm merged commit 03e5160 into airbytehq:master Dec 13, 2021
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
… connector (airbytehq#7945)

* Added cdk source

* Added free exchange rates api as a dotnet example

* Corrected the use of JsonDocument, properly enabled utf-8

* Fixed error catching

* Added cli, documentation and nuget improvements

* Small improvements where needed

* Removed obsolete files

* Update README.md

* Added documentation and reference

* Update README.md

* Corrected dotnet

* Update README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation community connectors/sources-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants