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

Finished Tap Refactor, upgraded all streams to v2 #77

Merged
merged 221 commits into from
May 17, 2022

Conversation

DownstreamDataTeam
Copy link
Contributor

Description of change

  • Finished refactoring the Tap, including code files (sync.py and the rest)
  • Upgraded all possible streams to v2 (some are still v1 as there is no alternative in v2)
  • Updated all schemas to match documentation on mambu.com
  • Added the schema extraction script to the repo, in case we need to use it in the future

Manual QA steps

Risks

  • Includes breaking changes

Rollback steps

  • revert this branch

Radu Marinoiu and others added 30 commits February 14, 2022 09:05
Pull changes from Stitch

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!32
release/37 changes, squashed

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!37
…e/38'

[ECDDC-575] Refactor Clients stream

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!40
…ream'

# Conflicts:
#   tap_mambu/sync.py
#   tap_mambu/tap_mambu_refactor/main.py
…e/38'

[ECDDC-555] Refactor centres stream

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!39
…ream' into 'release/38'

[ECDDC-604] Added field "assigned_branch_key" to clients stream

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!42
… into 'release/38'

[ECDDC-607] Changed readme file to correct tap_config.json into config.json

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!44
…elease/38'

[ECDDC-601] Add code scanning with sonar

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!43
Alexandru Rosca and others added 10 commits April 13, 2022 19:02
…/41'

[ECDDC-656] Refactor tap core code

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!94
[ECDDC-654] Added the Tap Tester into the CI pipeline

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!86
…/41'

[ECDDC-656] Renamed unittests folder and fixed an import bug

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!95
# Conflicts:
#	tap_mambu/tap_mambu_refactor/tap_processors/processor.py
# Conflicts:
#	tap_mambu/helpers/schemas/loan_transactions.json
#	tap_mambu/tap_processors/processor.py
Merge release/40 into release/41

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!100
@singer-bot
Copy link

Hi @DownstreamDataTeam, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@DownstreamDataTeam DownstreamDataTeam marked this pull request as ready for review April 26, 2022 16:28
Alexandru Rosca added 2 commits April 27, 2022 13:51
# Conflicts:
#	tap_mambu/tap_processors/audit_trail_processor.py
#	tap_mambu/tap_processors/processor.py
Merge master to release 41

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!105
Copy link
Contributor

@dmosorast dmosorast left a comment

Choose a reason for hiding this comment

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

Just one minor piece of feedback, but not a blocker since it'd be an enhancement. I'd like to do a bit more diffing tomorrow to better understand impact, but the code changes seem good to me. It's much cleaner now!

last_stream = None
if stream_name in selected_streams:
return True, last_stream
return False, last_stream
Copy link
Contributor

Choose a reason for hiding this comment

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

The get_selected_streams in the Catalog module will do a similar task to this, except it doesn't skip preceding streams if they're selected. Instead, it will shuffle the list of selected streams based on the currently syncing stream before returning the ordered set. https://github.com/singer-io/singer-python/blob/master/singer/catalog.py#L151

We've found that in practice, it's better to attempt all streams when resuming rather than wait for the next scheduled extraction, since sometimes folks have their frequency set at many hours or a whole day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll consider replacing this code with the included function

Copy link
Contributor

@dmosorast dmosorast left a comment

Choose a reason for hiding this comment

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

In my inspection of the JSON schema files today, I noticed one small thing that would be very hard to correct after release. It seems that the fields booking_date and creation_date are no longer marked with a "format": "date-time" Is this something you'd want to fix before I ship it? It might also be good to take some time to review for other date-times that need the format.

@dmosorast
Copy link
Contributor

In my inspection of the JSON schema files today, I noticed one small thing that would be very hard to correct after release. It seems that the fields booking_date and creation_date are no longer marked with a "format": "date-time" Is this something you'd want to fix before I ship it? It might also be good to take some time to review for other date-times that need the format.

@DownstreamDataTeam Sorry, that is for the interest_accrual_breakdown stream. Are these fields necessary to be typed as datetimes before releasing this as a major version?

Radu Marinoiu added 2 commits May 13, 2022 10:24
…ime' into 'release/41'

[ECDDC-719] Added format date-time to interest accrual breakdown

See merge request mambucom/product/ecosystem/mambu-marketplace/connectors/singer/tap-mambu!110
@dmosorast dmosorast merged commit ca3e61d into singer-io:master May 17, 2022
@dmosorast dmosorast mentioned this pull request May 17, 2022
@singer-bot
Copy link

You did it @DownstreamDataTeam!

Thank you for signing the Singer Contribution License Agreement.

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.

3 participants