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.

Sign up for GitHub

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

Jump to bottom

Recurly #1058

Merged

sherifnada merged 6 commits into airbytehq:master from yevhenii-ldv:recurly

Nov 25, 2020

Contributor

yevhenii-ldv commented Nov 23, 2020

What

Add source connector for Recurly to load data.

yevhenii-ldv added 3 commits

November 23, 2020 12:39


          First version of implementation source 'Recurly'

8ed90b9


          add documentation

24a9813


          update json schema files

75258f5

sherifnada suggested changes

View reviewed changes

Contributor

sherifnada left a comment

Small changes required but looks great so far.

airbyte-integrations/connectors/source-recurly/config.sample.json Outdated

		@@ -0,0 +1,4 @@
		{
		"access_token": "<your access token>",

Contributor

sherifnada Nov 23, 2020

could you place this in the sample_configs directory and update it to match the spec.json file?

airbyte-integrations/connectors/source-recurly/source_recurly/source.py Outdated

+                  ) -> Generator[AirbyteMessage, None, None]:
+                      client = self._client(config_container)
+                      catalog = AirbyteCatalog.parse_obj(self.read_config(catalog_path))

Contributor

sherifnada Nov 23, 2020

this object is now a ConfiguredAirbyteCatalog

Suggested change

      
                    catalog = AirbyteCatalog.parse_obj(self.read_config(catalog_path))
          
                    catalog = ConfiguredAirbyteCatalog.parse_obj(self.read_config(catalog_path))

Contributor

sherifnada Nov 23, 2020

See #1065 for an example of the necessary change here

airbyte-integrations/connectors/source-recurly/source_recurly/spec.json Outdated

+                  "properties": {
+                    "api_key": {
+                      "type": "string",
+                      "description": "Recurly API Key, use <a href=\"https://airbyte.recurly.com/integrations/api_keys\">admin</a> to generate this key."

Contributor

sherifnada Nov 23, 2020

current link points them to the Airbyte instance of recurly. Better to redirect to docs instead.

Suggested change

      
                    "description": "Recurly API Key, use <a href=\"https://airbyte.recurly.com/integrations/api_keys\">admin</a> to generate this key."
          
                    "description": "Recurly API Key. See the  <a href=\"https://docs.airbyte.io/integrations/sources/recurly\">docs</a> for more information on how to generate this key."

docs/integrations/sources/recurly.md Outdated


		### Output schema

		Several output streams are available from this source (accounts, plans, invoices, transactions, etc.).

Contributor

sherifnada Nov 23, 2020

Could you:

List the entities output (instead of "etc.")
Add a URL to the recurly docs that helps the user understand what each of these entities mean

Suggested change

      
            Several output streams are available from this source (accounts, plans, invoices, transactions, etc.).
          
            Several output streams are available from this source: 
          
            * accounts
          
            * ....

airbyte-integrations/connectors/source-recurly/source_recurly/client.py

+                      return streams
+                  def get_entities(self, entity_name):
+                      if entity_name == "export_dates":

Contributor

sherifnada Nov 23, 2020

Could you add a comment explaining why there is a special handling of this stream?

airbyte-integrations/connectors/source-recurly/source_recurly/client.py

+                          if resp.status_code == 200:
+                              entity_data = resp.json()["data"]
+                              while resp.json()["has_more"]:
+                                  resp = requests.get(f"{self.RECURLY_BASE_URL}{resp.json()['next']}", headers=self._headers)

Contributor

sherifnada Nov 23, 2020

is this correct? Recurly docs imply that the URL contained in next is complete and doesn't need to be appended to the base URL. To make sure that this is the case, is it possible to add a few hundred items to one of the streams just to future proof this? Ideally you can programmatically create 200 items in an entity via the API so it's not a ton of work on your end.

Contributor Author

yevhenii-ldv Nov 24, 2020

That's correct, added 200+ coupons to test pagination.

airbyte-integrations/connectors/source-recurly/source_recurly/client.py Outdated

+                          "Content-Type": "application/json",
+                      }
+                  def health_check(self):

Contributor

sherifnada Nov 23, 2020

Could you use return signatures on all methods e.g:

Suggested change

      
                def health_check(self):
          
                def health_check(self) -> Tuple[Boolean, object]:

airbyte-integrations/connectors/source-recurly/source_recurly/client.py Outdated

+                      self._client = RecurlyClient(api_key)
+                      self._headers = {
+                          "User-Agent": USER_AGENT,
+                          "Authorization": "Basic %s" % b64encode(bytes(api_key + ":", "ascii")).decode("ascii"),

Contributor

sherifnada Nov 23, 2020

Why is it necessary to do this instead of just adding API key? Could you add a comment to the code so the context is not lost?

airbyte-integrations/connectors/source-recurly/source_recurly/client.py Outdated

+                  PAGINATION = 100
+                  def __init__(self, api_key: str):
+                      self.ENTITY_MAP = ["accounts", "coupons", "invoices", "plans", "measured_units", "subscriptions", "transactions", "export_dates"]

Contributor

sherifnada Nov 23, 2020

nit: If this is just a list then rename to ENTITIES

airbyte-integrations/connectors/source-recurly/README.md

		@@ -0,0 +1,62 @@
		# Recurly Source

Contributor

sherifnada Nov 23, 2020

This readme looks out of date. Could you reference the latest template and update accordingly?

yevhenii-ldv added 3 commits

November 24, 2020 13:45


          update README.md, add comment into code

d9b93ff


          Merge branch 'master' of github.com:yevhenii-ldv/airbyte into recurly

09e95c1

� Conflicts:
�	docs/SUMMARY.md


          update Dockerfile, setup.py

042e678

sherifnada approved these changes

View reviewed changes

Contributor

sherifnada left a comment

Nice work!

sherifnada merged commit 9b03389 into airbytehq:master

snyk-bot mentioned this pull request

[Snyk] Security upgrade yup from 0.28.5 to 0.30.0 yeedster/airbyte#12

Open

snyk-bot mentioned this pull request

[Snyk] Fix for 5 vulnerabilities yeedster/airbyte#83

Open

yeedster mentioned this pull request

[Snyk] Fix for 5 vulnerabilities yeedster/airbyte#84

Open

snyk-bot mentioned this pull request

[Snyk] Fix for 5 vulnerabilities yeedster/airbyte#85

Open

This was referenced Oct 5, 2022

[Snyk] Fix for 5 vulnerabilities yeedster/airbyte#86

Open

[Snyk] Fix for 5 vulnerabilities yeedster/airbyte#87

Open

[Snyk] Fix for 5 vulnerabilities yeedster/airbyte#88

Open

[Snyk] Fix for 5 vulnerabilities yeedster/airbyte#89

Open

davydov-d added a commit that referenced this pull request


          #1058 source mailchimp: fix the way request params are build

2a55890

davydov-d added a commit that referenced this pull request


          #1058 source mailchimp: upd changelog

44327ff

davydov-d added a commit that referenced this pull request


          #1058 source mailchimp - rm unused field from spec

3e3f360

davydov-d added a commit that referenced this pull request


          #1058 source mailchimp: bypass expected records test

72706fd

davydov-d added a commit that referenced this pull request


          Merge branch 'master' into ddavydov/#1058-source-mailchimp-stream-sta…

…te-issue-second-try

davydov-d added a commit that referenced this pull request


          #1058 source mailchimp: add extra logs

ddd26bf

davydov-d added a commit that referenced this pull request


          #1058 source mailchimp: upd changelog

f20d532

davydov-d added a commit that referenced this pull request


          #1058 source mailchimp: flake fix

fe56860

davydov-d added a commit that referenced this pull request


          Merge branch 'master' into ddavydov/#1058-source-mailchimp-add-extra-…

be52e3f

…logs

davydov-d added a commit that referenced this pull request


          Source Mailchimp: add extra logs (#22228)

29cbc0a

* #1058 source mailchimp: add extra logs

* #1058 source mailchimp: upd changelog

* #1058 source mailchimp: flake fix

* auto-bump connector version

---------

Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

davydov-d added a commit that referenced this pull request


          #1058 source Mailchimp: revert extra logging

09eb2e8

davydov-d added a commit that referenced this pull request


          #1058 source Mailchimp: upd changelog

ca35835

davydov-d added a commit that referenced this pull request


          Source Mailchimp: revert extra logging (#22405)

ef164d1

* #1058 source Mailchimp: revert extra logging

* #1058 source Mailchimp: upd changelog

* auto-bump connector version

---------

Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet