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

Move postgres source to AB Protocol & various python fixes #610

Merged
merged 34 commits into from Oct 19, 2020

Conversation

sherifnada
Copy link
Contributor

@sherifnada sherifnada commented Oct 19, 2020

What

The main change:

  • Add a new source for tap-postgres that is ABP compliant

Supporting Python/singer fixes:

  • Output the correct messages from ExchangerateIO & Stripe sources for check connection
  • Always exit with statuscode 0 when checking connection unless an "actual" exception happened
  • Add helper methods to SingerHelper to parse from singer types to Airbyte types

Reading Order

Main change:

  1. postgres_singer_source/source.py
  2. SingerPostgresSourceTest.java
  3. remaining files in airbyte-integrations/singer/postgres_abprotocol

Supporting changes:

  1. */source.py
  2. entrypoint.py
  3. singer_helpers.py
  4. anything else

TODO

  • adapt to new directory structure
  • Use secrets instead of config
  • use SingerSource instead of Source
  • use proper JSONSchema instead of coercing the schema

@@ -0,0 +1 @@
config
Copy link
Contributor

Choose a reason for hiding this comment

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

See my PR this weekend: #606

@@ -34,6 +34,36 @@ class Catalogs:


class SingerHelper:
@staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

having multiple types is valid jsonschema. i don't think we want to normalize it out in the integration if we are saying that you define your stream's schema using jsonschema.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh. saw your comment here: #609 (review). maybe we're on the same page now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, we are. I was confused. JSONSchema it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which case please merge your PR 😆

@sherifnada sherifnada merged commit d4b6ebc into master Oct 19, 2020
@sherifnada sherifnada deleted the sherif/move-postgres-to-abprotocol branch October 19, 2020 21:36
@@ -85,19 +86,17 @@ def start(self, args):
if cmd == "check":
check_result = self.source.check(logger, config_container)

if check_result.successful:
if check_result.status == Status.SUCCEEDED:
Copy link
Contributor

Choose a reason for hiding this comment

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

@sherifnada If i understand right, this change is not compatible with what is currently in the template/python-source for the ´ check' function anymore, right?

AirbyteCheckResponse has to be changed to AirbyteConnectionStatus in that file too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChristopheDuong Correct - good catch

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.

None yet

5 participants