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

Source Stripe: add availability strategy #22659

Merged

Conversation

roman-yermilov-gl
Copy link
Collaborator

What

- turn availability strategy for all streams
- prepare custom availability strategy for substreams

@roman-yermilov-gl roman-yermilov-gl self-assigned this Feb 9, 2023
@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues connectors/source/stripe area/documentation Improvements or additions to documentation labels Feb 9, 2023
Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Solid - this sort of work would have handled the bug I ran into the first time around 😄 Please add a unit test!

docs/integrations/sources/stripe.md Outdated Show resolved Hide resolved
Comment on lines 24 to 26
available, reason = availability_strategy.check_availability(parent_stream_instance, logger, source)
if not available:
return available, reason
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
available, reason = availability_strategy.check_availability(parent_stream_instance, logger, source)
if not available:
return available, reason
is_available, reason = availability_strategy.check_availability(parent_stream_instance, logger, source)
if not is_available:
return is_available, reason

Small suggestion to make return is_available, reason not look on first glance that it's returning a true

Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Oops I meant to request changes for the test

Copy link
Contributor

@erohmensing erohmensing 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 unit tests! Last question is about empty streams

Comment on lines +22 to +26
empty_streams: ["bank_accounts", "checkout_sessions", "checkout_sessions_line_items", "external_account_bank_accounts"]
# TEST 1 - Reading catalog without invoice_line_items
- config_path: "secrets/config.json"
configured_catalog_path: "integration_tests/non_invoice_line_items_catalog.json"
empty_streams: ["transfers"]
Copy link
Contributor

Choose a reason for hiding this comment

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

What led to these increases in empty streams?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have new account for stripe. It was populated recently by our unblockers team. There are streams which they cannot populate right now for various reasons:

bank_accounts
external_account_bank_accounts
checkout_sessions
checkout_sessions_line_items
transfers

In order to not to block development of this connector we decided to create a new task to populate those streams later. I can do nothing with it for now

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for explaining, as long as that's being tracked!

@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented Feb 10, 2023

/test connector=connectors/source-stripe

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

Name                                     Stmts   Miss  Cover
------------------------------------------------------------
source_stripe/source.py                     22      0   100%
source_stripe/__init__.py                    2      0   100%
source_stripe/streams.py                   311     27    91%
source_stripe/availability_strategy.py      19      2    89%
------------------------------------------------------------
TOTAL                                      354     29    92%
	 Name                                                    Stmts   Miss  Cover   Missing
	 -------------------------------------------------------------------------------------
	 connector_acceptance_test/base.py                          12      4    67%   16-19
	 connector_acceptance_test/config.py                       141      5    96%   87, 93, 239, 243-244
	 connector_acceptance_test/conftest.py                     217    101    53%   37, 43-45, 50, 55, 78, 84, 90-92, 111, 116-118, 124-126, 132-133, 138-139, 144, 150, 159-168, 174-179, 194, 218, 249, 255, 263-271, 279-292, 300-313, 318-324, 331-342, 349-365
	 connector_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 connector_acceptance_test/tests/test_core.py              476    117    75%   53, 58, 97-108, 113-120, 124-125, 129-130, 380, 400, 438, 476-493, 506-517, 521-526, 532, 565-570, 608-615, 658-660, 663, 728-736, 748-751, 756, 812-813, 819, 822, 858-868, 881-906
	 connector_acceptance_test/tests/test_incremental.py       160     14    91%   58-65, 70-83, 246
	 connector_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 connector_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 connector_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 connector_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 connector_acceptance_test/utils/json_schema_helper.py     114     13    89%   31-32, 39, 42, 66-69, 97, 121, 203-205
	 -------------------------------------------------------------------------------------
	 TOTAL                                                    1696    347    80%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:98: The previous and actual specifications are identical.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:507: The previous and actual discovered catalogs are identical.
============ 53 passed, 3 skipped, 37 warnings in 315.59s (0:05:15) ============

@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented Feb 10, 2023

/publish connector=connectors/source-stripe

🕑 Publishing the following connectors:
connectors/source-stripe
https://github.com/airbytehq/airbyte/actions/runs/4144597480


Connector Did it publish? Were definitions generated?
connectors/source-stripe

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@airbyteio airbyteio temporarily deployed to more-secrets February 10, 2023 14:27 — with GitHub Actions Inactive
@airbyteio airbyteio temporarily deployed to more-secrets February 10, 2023 14:27 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Airbyte Code Coverage

There is no coverage information present for the Files changed

Total Project Coverage 24.67%

@roman-yermilov-gl roman-yermilov-gl force-pushed the ryermilov/source-stripe-add-availability-strategy branch from cd85519 to b93c1fe Compare February 13, 2023 16:40
@roman-yermilov-gl
Copy link
Collaborator Author

/approve-and-merge reason="published before code freeze"

@octavia-approvington
Copy link
Contributor

This looks fine!
Merged!
Imagine it being fine

@octavia-approvington octavia-approvington merged commit 1d406ef into master Feb 14, 2023
@octavia-approvington octavia-approvington deleted the ryermilov/source-stripe-add-availability-strategy branch February 14, 2023 08:28
@lazebnyi lazebnyi linked an issue Feb 14, 2023 that may be closed by this pull request
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 connectors/source/stripe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Stripe: Enable AvailabilityStrategy
6 participants