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 Source: Chartmogul #9381

Merged
merged 12 commits into from
Jan 18, 2022
Merged

Conversation

TSkrebe
Copy link
Contributor

@TSkrebe TSkrebe commented Jan 10, 2022

What

This PR adds a new source: chartmogul api

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.


This change is Reviewable

@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Jan 10, 2022
@TSkrebe
Copy link
Contributor Author

TSkrebe commented Jan 10, 2022

./acceptance-test-docker.sh

 test_core.py ✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                                                                                                                                        93% █████████▍
 test_full_refresh.py ✓                                                                                                                                                                                           100% ██████████
==================================================================================================== short test summary info ====================================================================================================
SKIPPED [1] source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config

Results (73.78s):
      14 passed

@TSkrebe TSkrebe marked this pull request as ready for review January 10, 2022 13:52
@TSkrebe
Copy link
Contributor Author

TSkrebe commented Jan 10, 2022

Not sure how to proceed with airbyte-integrations/builds.md

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

The connector looks well implemented, I created one issue to get credentials to apply CI tests; If possible to share a testing credentials this helps to merge sooner.

@marcosmarxm
Copy link
Member

Not sure how to proceed with airbyte-integrations/builds.md

The link is built after the connector is merged.

@alafanechere alafanechere self-assigned this Jan 11, 2022
@TSkrebe
Copy link
Contributor Author

TSkrebe commented Jan 13, 2022

i'm in no rush, just curious on ETA when we could see this published?

@alafanechere
Copy link
Contributor

Hi @TSkrebe, we created a sandbox account for Chartmogul. I'll now open a side branch to run the acceptance tests on this account. If they pass I'll go for a review, and if I've no additional change request we can publish it today the beginning of next week.

@alafanechere alafanechere mentioned this pull request Jan 14, 2022
@alafanechere alafanechere temporarily deployed to more-secrets January 14, 2022 09:09 Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 14, 2022 09:10 Inactive
@alafanechere
Copy link
Contributor

alafanechere commented Jan 14, 2022

Hi @TSkrebe I made some minor changes to make the acceptance tests pass:

  • Run ./gradlew format
  • Fix some unused import to make Flake check pass
  • Extensive use of mocker fixture in unit tests to replace mock.MagicMock with mocker.MagicMock
  • Change customer id field to integer as it's the type the API returned
  • Make primary key fields not nullable in streams schema. Your schemas have a lot of nullable fields, please double-check they are all really nullable according to Chartmogul API reference.
  • Declare activities as an empty stream in basic_read acceptance test. Our sandbox account does not have any activity. Do you know how we could generate an activity? I'm not familiar with Chartmogul.

I highlighted the changes on which I'd like a reply from your side.

@alafanechere alafanechere temporarily deployed to more-secrets January 14, 2022 09:47 Inactive
@alafanechere
Copy link
Contributor

Not sure how to proceed with airbyte-integrations/builds.md

You have to add a line for your connector at the right alphabetical place and copy what's been made for other connectors.
I did it in bd82338

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Thank you @TSkrebe, the overall code looks good to me. I still have some minor suggestions. Let me know what you think.

@TSkrebe
Copy link
Contributor Author

TSkrebe commented Jan 14, 2022

@alafanechere

Change customer id field to integer as it's the type the API returned

makes sense, don't why it was a string. thanks

Declare activities as an empty stream in basic_read acceptance test. Our sandbox account does not have any activity. Do you know how we could generate an activity? I'm not familiar with Chartmogul.

in my case, there is integration with stripe https://app.chartmogul.com/#/data-platform/data-sources . Whenever there is a payment, a new activity is created. If there is any activities in your stripe sandbox it should theoretically work.

Make primary key fields not nullable in streams schema. Your schemas have a lot of nullable fields, please double-check they are all really nullable according to Chartmogul API reference.

Since they don't provide open api, it's difficult to know which ones can and can't be null.

thanks for the feedback on python style. Difficult to switch from golang midset... will address after the weekend.

@TSkrebe
Copy link
Contributor Author

TSkrebe commented Jan 17, 2022

@alafanechere addressed your comments. let me know if i missed anything. you'll probably need to run gradlew command again as i wasn't able to get it up-and-running on my M1

@alafanechere alafanechere temporarily deployed to more-secrets January 18, 2022 16:12 Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 18, 2022 16:13 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 18, 2022 16:29 Inactive
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Thanks for the changes you made. I confirm that acceptance test are passing locally for me after I made some unused import removal that blocked the flake check.
I'm going to make the test pass in the CI on my side branch + publish the connector, then we'll be ready to merge.

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 18, 2022 16:39 Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 18, 2022 16:59 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 18, 2022 18:24 Inactive
@alafanechere alafanechere merged commit 0815100 into airbytehq:master Jan 18, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants