🎉 New Source Chargify: first connector #10853

Javier162380 · 2022-03-04T16:04:39Z

What

First Draft of a Chargify connector for subscriptions and customers objects

How

I use the Python Http template API, for developing the skeleton of the connector, and also I take to the Greenhouse and Zendesk sources

🚨 User Impact 🚨

No breaking changes it is a new connector.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

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

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

CLAassistant · 2022-03-04T16:04:45Z

All committers have signed the CLA.

…airbyte into feature/chargify-connector

alafanechere · 2022-03-04T17:21:54Z

Hey @Javier162380, thanks for this contrib! Is this ready for review? (The PR title is a bit misleading as you wrote it's a draft). If it's still WIP please convert the PR to draft 🙏 .

Javier162380 · 2022-03-07T10:59:18Z

Hey @Javier162380, thanks for this contrib! Is this ready for review? (The PR title is a bit misleading as you wrote it's a draft). If it's still WIP please convert the PR to draft 🙏 .

Hey, thanks for the answer @alafanechere! The connector is ready to be reviewed!. Sadly I can not add integration tests, but the unit test code coverage covers almost all the edge cases 😊. Shall I remove all this all this files from the PR or shall I leave them there

alafanechere

Hi @Javier162380 I went for a first review.
I made small suggestions about your current implementation, mainly to ease readability.
I'd also ask you to edit the following files:

airbyte-config/init/src/main/resources/seed/source_definitions.yaml to add your connector. Please locally generate a UUID (in bash: uuidgen | tr "[:upper:]" "[:lower:]") and use this as you definition id.
Add your connector to airbyte-integrations/builds.md (just copy another URL and edit it to use source-chargify
Could you please add a documentation for this connector in docs/integrations/sources/chargify.md

Could you explain why you can't run the acceptance tests?
We need to have a way to run these in our CI. We usually create sandbox account with the source service, but I discovered that Chargify exposes a mock server for API testing that we could use if you change the connector config a little bit (using a domain parameter rather than a subdomain).

alafanechere · 2022-03-09T08:06:51Z

airbyte-integrations/connectors/source-chargify/acceptance-test-config.yml

+# TODO uncomment this block to specify that the tests should assert the connector outputs the records provided in the input file a file
+#      expect_records:
+#        path: "integration_tests/expected_records.txt"
+#        extra_fields: no
+#        exact_order: no
+#        extra_records: yes


Please implement or remove all the remaining #TODO.

alafanechere · 2022-03-09T08:08:13Z

airbyte-integrations/connectors/source-chargify/acceptance-test-config.yml

+#        extra_fields: no
+#        exact_order: no
+#        extra_records: yes
+  incremental: # TODO if your connector does not implement incremental sync, remove this block


Your connector does not have incremental sync, please remove this test.

alafanechere · 2022-03-09T08:08:35Z

airbyte-integrations/connectors/source-chargify/integration_tests/abnormal_state.json

@@ -0,0 +1,5 @@
+{


Your connector does not implement incremental sync, please remove this file.

alafanechere · 2022-03-09T08:09:44Z

airbyte-integrations/connectors/source-chargify/integration_tests/catalog.json

@@ -0,0 +1,39 @@
+{


Please fill this file according to the output of running discover on the connector.

alafanechere · 2022-03-09T08:11:55Z

airbyte-integrations/connectors/source-chargify/integration_tests/sample_config.json

@@ -0,0 +1,3 @@
+{


Please fill this file according to a valid config.

change I use the spotlight endpoint f493f40

alafanechere · 2022-03-09T08:39:21Z

airbyte-integrations/connectors/source-chargify/source_chargify/source.py

+        if next_page_token is None and self.is_first_requests is True:
+            self.is_first_requests == False
+            return {"page": self._page, "per_page": self._per_page}


I don't think self.is_first_requests is an useful attribute. next_page_token == None means you're making the first request. Moreover, changing a property value could lead to unexpected behavior. If you want to keep is_first_request please declare this attribute in the constructor, not in a property.

Suggested change

if next_page_token is None and self.is_first_requests is True:

self.is_first_requests == False

return {"page": self._page, "per_page": self._per_page}

if next_page_token is None:

return {"page": self._page, "per_page": self._per_page}

More clear sure! changes applied!

alafanechere · 2022-03-09T08:41:24Z

airbyte-integrations/connectors/source-chargify/source_chargify/source.py

+        TODO: Override this method to define how a response is parsed.
+        :return an iterable containing each record in the response
+        """
+        yield {}


You need to implement this method to extract records from your response object, otherwise, I think the connector won't output any data.

I don't think so this interface it is a base stream, I am using a custom logic per object to parse the response and in those interfaces it is where I implement this method.

alafanechere · 2022-03-09T08:44:03Z

airbyte-integrations/connectors/source-chargify/source_chargify/source.py

+            converted_args = self.convert_config2stream_args(config)
+            customers_gen = Customers(authenticator=auth, **converted_args).read_records(sync_mode=SyncMode.full_refresh)


Suggested change

converted_args = self.convert_config2stream_args(config)

customers_gen = Customers(authenticator=auth, **converted_args).read_records(sync_mode=SyncMode.full_refresh)

customers_gen = Customers(authenticator=auth, subdomain=config["subdomain"]).read_records(sync_mode=SyncMode.full_refresh)

I don't think convert_config2stream_args. is useful here.

Could you also try to read records from both streams in the check connection? To make sure we can read all the streams defined in the connector.

yeah make sense I erase the method 2420d9b

Method erase 2420d9b, I can read records from both streams

You're still calling it on line 105

alafanechere · 2022-03-09T08:48:05Z

airbyte-integrations/connectors/source-chargify/unit_tests/test_source.py

+    source = SourceChargify()
+    config_mock = {"subdomain": "tst", "page": 1, "per_page": 200}
+    streams_config = source.convert_config2stream_args(config_mock)
+    assert streams_config == {"subdomain":"tst", "page": 1, "per_page": 200}


Missing new line at the end of the file.

do we have a python linter for the project?

alafanechere · 2022-03-09T08:51:12Z

airbyte-integrations/connectors/source-chargify/source_chargify/spec.json

+        "description": "Chargify API Key.",
+        "airbyte_secret": true
+      },
+      "subdomain": {


Do you think you could change this to domain? It will allow us to use Chargify's mock server to run our acceptance tests:

curl --request GET \ --url https://stoplight.io/mocks/chargify/api-docs/14108261/customers.json \ --header 'Authorization: Basic undefined' \ --header 'Content-Type: application/json'

I change the attribute to use the domain

Javier162380 · 2022-03-10T13:49:04Z

Hi @Javier162380 I went for a first review. I made small suggestions about your current implementation, mainly to ease readability. I'd also ask you to edit the following files:

airbyte-config/init/src/main/resources/seed/source_definitions.yaml to add your connector. Please locally generate a UUID (in bash: uuidgen | tr "[:upper:]" "[:lower:]") and use this as you definition id.

Add your connector to airbyte-integrations/builds.md (just copy another URL and edit it to use source-chargify

Could you please add a documentation for this connector in docs/integrations/sources/chargify.md

Could you explain why you can't run the acceptance tests? We need to have a way to run these in our CI. We usually create sandbox account with the source service, but I discovered that Chargify exposes a mock server for API testing that we could use if you change the connector config a little bit (using a domain parameter rather than a subdomain).

Hi @alafanechere,

Thanks for you review!

I applied the changes you mention and I test locally the connector using the Chargify mock instance. It works as expected, let me know if more work it is required

alafanechere

Thanks for the changes! I still spotted some formatting issues. Could you please run ./gradlew format ?
Could you please also share the output of the acceptance tests?

alafanechere · 2022-03-10T15:42:37Z

airbyte-integrations/connectors/source-chargify/source_chargify/spec.json

+        "description": "Chargify API Key.",
+        "airbyte_secret": true
+      },
+      "domain": {


Suggested change

"domain": {

"server": {

The API doc uses the word server not domain 😄

alafanechere · 2022-03-10T16:41:32Z

Hey @Javier162380 , I made some quick changes to make this move forward (formatting mainly).
I ran the acceptance tests with the following config but some are failing (during schema validation).
I used the following secrets/config.json, feel free to use the same:

{
    "api_key": "undefined",
    "domain": "stoplight.io/mocks/chargify/api-docs/14108261/"
}

I think one of your unit test is failing too.

Please fix the acceptance and unit tests and ping me for another review.

alafanechere · 2022-03-16T15:51:44Z

/test connector=connectors/source-chargify

🕑 connectors/source-chargify https://github.com/airbytehq/airbyte/actions/runs/1993951288
✅ connectors/source-chargify https://github.com/airbytehq/airbyte/actions/runs/1993951288
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              275    106    61%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  876    259    70%
Name                          Stmts   Miss  Cover
-------------------------------------------------
source_chargify/__init__.py       2      0   100%
source_chargify/source.py        67     11    84%
-------------------------------------------------
TOTAL                            69     11    84%

Python short test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.7/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
======================== 20 passed, 1 skipped in 18.97s ========================

alafanechere

@Javier162380 I made some small changes to make the acceptance pass and for your PR to be ready for merge. You copied the doc from the greenhouse connector so I wrote a simple one myself for Chargify. Please check my changes and feel free to add more details to the documentation.

Acceptance tests are passing locally for me, I'll run them in the CI and merge once they pass.

Javier162380 · 2022-03-17T09:12:54Z

@Javier162380 I made some small changes to make the acceptance pass and for your PR to be ready for merge. You copied the doc from the greenhouse connector so I wrote a simple one myself for Chargify. Please check my changes and feel free to add more details to the documentation.

Acceptance tests are passing locally for me, I'll run them in the CI and merge once they pass.

Hi @alafanechere ,

Sorry I was away for a couple of days!! Your changes look good to me!!. I will love to see this connector publish 🤟

alafanechere · 2022-03-17T09:32:15Z

/publish connector=connectors/source-chargify run-tests=false

🕑 connectors/source-chargify https://github.com/airbytehq/airbyte/actions/runs/1997741018
✅ connectors/source-chargify https://github.com/airbytehq/airbyte/actions/runs/1997741018

alafanechere · 2022-03-17T10:30:26Z

@Javier162380 thank you for this new connector!

Javier162380 · 2022-03-17T11:15:20Z

thanks for your help @alafanechere!!

first connector draft

71b8ad1

github-actions bot added the area/connectors Connector related issues label Mar 4, 2022

octavia-squidington-iii added the community label Mar 4, 2022

Javier162380 added 3 commits March 4, 2022 17:05

Delete .python-version

32e9f3a

solve chargify source pagination issues'

0db52f6

Merge branch 'feature/chargify-connector' of github.com:Javier162380/…

9e2f236

…airbyte into feature/chargify-connector

alafanechere changed the title ~~first connector draft~~ Source Chargify: first connector draft Mar 4, 2022

alafanechere self-assigned this Mar 4, 2022

adding test coverage for chargify

a8fb5f8

Javier162380 changed the title ~~Source Chargify: first connector draft~~ Source Chargify: first connector Mar 7, 2022

alafanechere mentioned this pull request Mar 9, 2022

CI Sandbox for Chargify #10969

Closed

alafanechere linked an issue Mar 9, 2022 that may be closed by this pull request

CI Sandbox for Chargify #10969

Closed

alafanechere requested changes Mar 9, 2022

View reviewed changes

apply alafane feedback

f493f40

github-actions bot added the area/documentation Improvements or additions to documentation label Mar 10, 2022

Javier162380 added 3 commits March 10, 2022 13:57

modifying customer schema

5d3b3e9

simplifying source

2420d9b

solving pagination issues

900d5e0

alafanechere requested changes Mar 10, 2022

View reviewed changes

alafanechere reviewed Mar 10, 2022

View reviewed changes

format

afdb2b4

Javier162380 added 2 commits March 11, 2022 12:15

solving unit testing

ac5788a

solving integration tests typo

c0c433c

marcosmarxm changed the title ~~Source Chargify: first connector~~ 🎉 New Source Chargify: first connector Mar 14, 2022

fix schemas

94276a0

alafanechere added 3 commits March 16, 2022 16:40

cleaning + refacto

f4e1f7f

add chargify to source_definitions and builds.md

8cbed6a

edit doc

7506a80

alafanechere changed the base branch from master to 0.29.12-alpha March 16, 2022 15:51

alafanechere changed the base branch from 0.29.12-alpha to master March 16, 2022 15:51

alafanechere approved these changes Mar 16, 2022

View reviewed changes

alafanechere added 2 commits March 17, 2022 10:28

Merge branch 'master' into feature/chargify-connector

a3149f7

set doc url in spec

6c3a9c1

update source_specs

3fe2869

alafanechere merged commit 4684476 into airbytehq:master Mar 17, 2022

octavia-squidington-iii mentioned this pull request Mar 17, 2022

Bump Airbyte version from 0.35.56-alpha to 0.35.57-alpha #11243

Merged

marcosmarxm mentioned this pull request Nov 30, 2022

New Source Connector: Chargify #10792

Closed

		converted_args = self.convert_config2stream_args(config)
		customers_gen = Customers(authenticator=auth, **converted_args).read_records(sync_mode=SyncMode.full_refresh)

🎉 New Source Chargify: first connector #10853

🎉 New Source Chargify: first connector #10853

Conversation

Javier162380 commented Mar 4, 2022

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

CLAassistant commented Mar 4, 2022 • edited Loading

alafanechere commented Mar 4, 2022

Javier162380 commented Mar 7, 2022

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Javier162380 Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Javier162380 commented Mar 10, 2022

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere commented Mar 10, 2022

alafanechere commented Mar 16, 2022 • edited by github-actions bot Loading

alafanechere left a comment

Choose a reason for hiding this comment

Javier162380 commented Mar 17, 2022

alafanechere commented Mar 17, 2022 • edited by github-actions bot Loading

alafanechere commented Mar 17, 2022

Javier162380 commented Mar 17, 2022

CLAassistant commented Mar 4, 2022 •

edited

Loading

Javier162380 Mar 10, 2022 •

edited

Loading

alafanechere commented Mar 16, 2022 •

edited by github-actions bot

Loading

alafanechere commented Mar 17, 2022 •

edited by github-actions bot

Loading