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.

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

🎉 New Source: Harvest #3709

Merged
merged 23 commits into from
Jun 7, 2021
Merged

🎉 New Source: Harvest #3709

merged 23 commits into from
Jun 7, 2021

Conversation

htrueman
Copy link
Contributor

@htrueman htrueman commented May 28, 2021 •
edited
Loading

What

Closes #2134

How

  • Added source Harvest integration.
  • Implemented all streams available.
  • In most cases there are incremental stream support.
    • Not available for Company stream (as it always has just a single record)
    • Not available for Reports API streams, as they don't have any possible cursor fields.

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. Schemas and configs in source_harvest/schemas/ and integration_tests/
  2. source_harvest python modules.
  3. Docs and source definitions in source root and common docs dir.
  4. the rest

@htrueman
Add harvest source.
48f31de 
Add Clients, Contacts, Company, Invoices, InvoiceMessages, InvoicePayments streams full_refresh support.
@htrueman
Add all Harvest source streams.
9da8442 
Add config and schema files for all the streams.
@htrueman
Complete streams implementation.
2dbaebc 
Add config and schemas.
@htrueman
Fix expenses stream calls.
76e8af2 
Fix ExpensesBase request_params method.
Add source docs and definitions.
@htrueman htrueman self-assigned this May 28, 2021
@auto-assign auto-assign bot requested review from jrhizor and sherifnada May 28, 2021 13:21
@htrueman
Copy link
Contributor Author

htrueman commented May 31, 2021 •
edited by github-actions bot
Loading

/test connector=source-harvest

🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/892435359
❌ source-harvest https://github.com/airbytehq/airbyte/actions/runs/892435359

@htrueman
Copy link
Contributor Author

htrueman commented May 31, 2021 •
edited by github-actions bot
Loading

/test connector=source-harvest

🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/893257009
❌ source-harvest https://github.com/airbytehq/airbyte/actions/runs/893257009
🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/893257009
❌ source-harvest https://github.com/airbytehq/airbyte/actions/runs/893257009

@htrueman
Copy link
Contributor Author

htrueman commented May 31, 2021 •
edited by github-actions bot
Loading

/test connector=source-harvest

🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/893341250
✅ source-harvest https://github.com/airbytehq/airbyte/actions/runs/893341250

keu
keu requested changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@keu keu left a comment

Choose a reason for hiding this comment

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

see my comments

@htrueman
Refactor streams.py code common usages.
5780fc5 
Update HarvestTokenAuthenticator class definition.
@htrueman htrueman requested a review from keu June 2, 2021 19:33
keu
keu approved these changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@keu keu left a comment

Choose a reason for hiding this comment

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

lgtm, just small comments, up to you

@keu keu requested a review from sherifnada June 2, 2021 20:35
sherifnada
sherifnada suggested changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

Looks great, nicely done Vadym 🎉 one blocking question about reports and a few smaller questions

docs/integrations/sources/harvest.md Outdated Show resolved Hide resolved
docs/integrations/sources/harvest.md Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-harvest/source_harvest/source.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-harvest/source_harvest/spec.json Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-harvest/source_harvest/streams.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-harvest/source_harvest/streams.py
current_date = pendulum.now()
# `from` and `to` params are required for reports calls
# min `from` value is current_date - 1 year
params.update({"from": self._from_date.strftime("%Y%m%d"), "to": current_date.strftime("%Y%m%d")})
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to make this incremental by manipulating the from and to params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have a reason for that. Discussed there.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can enrich the record returned from the API with this info by doing the following:

  1. inside parse_response we can access the original request object that was made then parse the from and to parameters.
  2. put those properties in the record response i.e: record['from'] = ...
  3. parse inside the get_updated_state method
  4. profit
    WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

this would require changing the schema 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.

It makes sense for me. I'll try to do it.

@htrueman
Add streams docstring.
8ea3c76 
Fix sub streams slicing.
Update spec and schemas.
Update md docs.
@htrueman
Copy link
Contributor Author

htrueman commented Jun 3, 2021 •
edited by github-actions bot
Loading

/test connector=source-harvest

🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/902530946
❌ source-harvest https://github.com/airbytehq/airbyte/actions/runs/902530946
🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/902530946
❌ source-harvest https://github.com/airbytehq/airbyte/actions/runs/902530946
🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/902530946
✅ source-harvest https://github.com/airbytehq/airbyte/actions/runs/902530946

@htrueman
Copy link
Contributor Author

htrueman commented Jun 4, 2021 •
edited by github-actions bot
Loading

/test connector=source-harvest

🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/906570164
❌ source-harvest https://github.com/airbytehq/airbyte/actions/runs/906570164

@htrueman
Copy link
Contributor Author

htrueman commented Jun 4, 2021 •
edited by github-actions bot
Loading

/test connector=source-harvest

🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/906784601
❌ source-harvest https://github.com/airbytehq/airbyte/actions/runs/906784601
🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/906784601
✅ source-harvest https://github.com/airbytehq/airbyte/actions/runs/906784601

sherifnada
sherifnada approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

LGTM with one question about cursor field. Nicely done Vadym! Incremental reports is great to have

airbyte-integrations/connectors/source-harvest/source_harvest/streams.py Outdated Show resolved Hide resolved
@htrueman
Fix streams cursor value.
ea519e3 
Update IncrementalReportsBase request_params.
Update schemas and config.
@htrueman
Copy link
Contributor Author

htrueman commented Jun 5, 2021 •
edited by github-actions bot
Loading

/test connector=source-harvest

🕑 source-harvest https://github.com/airbytehq/airbyte/actions/runs/909909337
✅ source-harvest https://github.com/airbytehq/airbyte/actions/runs/909909337

@htrueman
Merge remote-tracking branch 'origin/master' into htrueman/source-har…
9dc473d 
…vest

# Conflicts:
#	airbyte-config/init/src/main/resources/seed/source_definitions.yaml
@htrueman
Add harvest source definitions.
261790d
@htrueman
Copy link
Contributor Author

htrueman commented Jun 7, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-harvest

🕑 connectors/source-harvest https://github.com/airbytehq/airbyte/actions/runs/914705974
✅ connectors/source-harvest https://github.com/airbytehq/airbyte/actions/runs/914705974

@htrueman htrueman merged commit e699f39 into master Jun 7, 2021
@htrueman htrueman deleted the htrueman/source-harvest branch June 7, 2021 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keu keu keu approved these changes

@sherifnada sherifnada sherifnada approved these changes

@yevhenii-ldv yevhenii-ldv yevhenii-ldv approved these changes

@vitaliizazmic vitaliizazmic Awaiting requested review from vitaliizazmic

@Janardhanpoola Janardhanpoola Awaiting requested review from Janardhanpoola

Assignees

@htrueman htrueman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New source: Harvest
5 participants
@htrueman @keu @sherifnada @yevhenii-ldv @Janardhanpoola