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: Strava #7151

Merged
merged 19 commits into from
Oct 26, 2021
Merged

🎉 New Source: Strava #7151

merged 19 commits into from
Oct 26, 2021

Conversation

terencecho
Copy link
Contributor

@terencecho terencecho commented Oct 19, 2021

What

  • Add new source: Strava

How

Add two streams for this source:

  • Athlete Stats
  • Activities

Recommended reading order

  1. bootstrap.md
  2. docs/integrations/sources/strava.md
  3. spec.json
  4. Schemas under schemas/*.json
  5. source.py
  6. streams.py

Tests

Unit Tests
(.venv) ❰tcho❙~/code/forks/airbyte/airbyte-integrations/connectors/source-strava(git✱source-strava)❱✔≻ python3 -m pytest -s unit_tests
Test session starts (platform: darwin, Python 3.9.7, pytest 6.2.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /Users/tcho/code/forks/airbyte, configfile: pytest.ini
plugins: sugar-0.9.4, mock-3.6.1, timeout-1.4.2
collecting ...
 ../../airbyte/airbyte-integrations/connectors/source-strava/unit_tests/test_incremental_streams.py::test_cursor_field ✓                                                                                                         7% ▋
 ../../airbyte/airbyte-integrations/connectors/source-strava/unit_tests/test_incremental_streams.py::test_get_updated_state ✓                                                                                                   13% █▍
 ../../airbyte/airbyte-integrations/connectors/source-strava/unit_tests/test_incremental_streams.py::test_stream_slices ✓                                                                                                       20% ██
 ../../airbyte/airbyte-integrations/connectors/source-strava/unit_tests/test_incremental_streams.py::test_supports_incremental ✓                                                                                                27% ██▋
 ../../airbyte/airbyte-integrations/connectors/source-strava/unit_tests/test_incremental_streams.py::test_source_defined_cursor ✓                                                                                               33% ███▍
 ../../airbyte/airbyte-integrations/connectors/source-strava/unit_tests/test_incremental_streams.py::test_stream_checkpoint_interval ✓                                                                                          40% ████
 airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py::test_request_params ✓                                                                                                                                47% ████▋
 airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py::test_next_page_token ✓                                                                                                                               53% █████▍
 airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py::test_request_headers ✓                                                                                                                               60% ██████
 airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py::test_http_method ✓                                                                                                                                   67% ██████▋
 airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py::test_should_retry[HTTPStatus.OK-False] ✓                                                                                                             73% ███████▍
 airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py::test_should_retry[HTTPStatus.BAD_REQUEST-False] ✓                                                                                                    80% ████████
 airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py::test_should_retry[HTTPStatus.TOO_MANY_REQUESTS-True] ✓                                                                                               87% ████████▋
 airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py::test_should_retry[HTTPStatus.INTERNAL_SERVER_ERROR-True] ✓                                                                                           93% █████████▍
 airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py::test_backoff_time ✓                                                                                                                                 100% ██████████
============================================================================================================== warnings summary ==============================================================================================================
airbyte-integrations/connectors/source-strava/unit_tests/test_incremental_streams.py: 6 warnings
airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py: 9 warnings
  /Users/tcho/code/forks/airbyte/airbyte-integrations/connectors/source-strava/.venv/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/http.py:37: DeprecationWarning: Call to deprecated class NoAuth. (Set `authenticator=None` instead) -- Deprecated since version 0.1.20.
    self._authenticator = NoAuth()

airbyte-integrations/connectors/source-strava/unit_tests/test_incremental_streams.py: 6 warnings
airbyte-integrations/connectors/source-strava/unit_tests/test_streams.py: 9 warnings
  /Users/tcho/code/forks/airbyte/airbyte-integrations/connectors/source-strava/.venv/lib/python3.9/site-packages/deprecated/classic.py:173: DeprecationWarning: Call to deprecated class HttpAuthenticator. (Use requests.auth.AuthBase instead) -- Deprecated since version 0.1.20.
    return old_new1(cls, *args, **kwargs)

-- Docs: https://docs.pytest.org/en/stable/warnings.html

Results (0.25s):
      15 passed
Integration Tests
(.venv) ❰tcho❙~/code/forks/airbyte/airbyte-integrations/connectors/source-strava(git✱source-strava)❱✔≻ python -m pytest -p integration_tests.acceptance
Test session starts (platform: darwin, Python 3.9.7, pytest 6.2.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /Users/tcho/code/forks/airbyte, configfile: pytest.ini
plugins: sugar-0.9.4, mock-3.6.1, timeout-1.4.2
collecting ...
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_match_expected[inputs0] ✓                                                                              7% ▊
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_required[inputs0] ✓                                                                                   14% █▌
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_optional[inputs0] ✓                                                                                   21% ██▎
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_has_secret[inputs0] ✓                                                                                 29% ██▉
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_secret_never_in_the_output[inputs0] ✓                                                                 36% ███▋
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_oauth_flow_parameters[inputs0] ✓                                                                      43% ████▍
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs0] ✓                                                                                50% █████
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs1] ✓                                                                                57% █████▊
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_discover[inputs0] ✓                                                                              64% ██████▌
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_defined_cursors_exist_in_schema[inputs0] ✓                                                       71% ███████▎
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestBasicRead.test_read[inputs0] ✓                                                                                  79% ███████▉
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_full_refresh.py::TestFullRefresh.test_sequential_reads[inputs0] ✓                                                            86% ████████▋
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py::TestIncremental.test_two_sequential_reads[inputs0] ✓                                                         93% █████████▍
 ../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py::TestIncremental.test_state_with_abnormally_large_values[inputs0] s                                          100% ██████████
========================================================================================================== short test summary info ===========================================================================================================
SKIPPED [1] ../../../../../airbyte/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py:21: `future_state_path` not specified, skipping

Results (27.68s):
      13 passed
       1 skipped

Pre-merge Checklist

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
  • Connector added to connector index like described here

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

@CLAassistant
Copy link

CLAassistant commented Oct 19, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Oct 19, 2021
@terencecho terencecho marked this pull request as ready for review October 20, 2021 01:38
@terencecho
Copy link
Contributor Author

Am I supposed to generate a sourceDefinitionId to add to the connector index myself, in accordance to the last item on the pre-merge checklist for contributors? The documentation linked only refers to updating <uuid>.json files that already exists when updating connectors.

@marcosmarxm
Copy link
Member

Am I supposed to generate a sourceDefinitionId to add to the connector index myself, in accordance to the last item on the pre-merge checklist for contributors? The documentation linked only refers to updating <uuid>.json files that already exists when updating connectors.

Yes.

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.

Hi @terencecho I made some comments about the contribution, is it possible to share the integration credentials to test (you can send to me in Slack). I request to another member to review your code in more details.

If possible add the endpoint docs in each stream.

@terencecho
Copy link
Contributor Author

@marcosmarxm Thank you for the comments! I will address your commends and share the integration credentials to you through slack.

If possible add the endpoint docs in each stream.

I added endpoint docs in comments underneath each stream class such that they look like:

class Activities(IncrementalStravaStream):
    """
    Returns the activities of an athlete.
    API Docs: https://developers.strava.com/docs/reference/#api-Activities-getLoggedInAthleteActivities
    Endpoint: https://www.strava.com/api/v3/athlete/activities
    """

Is there another place I should add that information to as well?

"athlete": {
"type": "object",
"properties": {
"MetaAthlete": {
Copy link
Member

Choose a reason for hiding this comment

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

remove this, is not saving the correct data where only need id/resource_state field.
image

Copy link
Member

Choose a reason for hiding this comment

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

I think you need to correct to all nested objects from this schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make those changes. The reason I added them in the first place is because the output of the api is an "array of SummaryActivity objects" along with the other nested objects.

@marcosmarxm
Copy link
Member

Overall after those changes the connector is ready to merge 🎉

@terencecho
Copy link
Contributor Author

@marcosmarxm Thanks for the extra comments. They should all be addressed now. Let me know if I understood any of your comments incorrectly, especially regarding the schema json files.

@terencecho
Copy link
Contributor Author

All unit/integration tests are passing and got it running locally with no problems. should be good for final look over.

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.

LGTM awesome work @terencecho

@marcosmarxm
Copy link
Member

@terencecho just to confirm, run ./gradlew format to format all files and check flake/linting.

terencecho and others added 3 commits October 25, 2021 20:28
Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
@terencecho
Copy link
Contributor Author

@marcosmarxm thanks for your help. I decided to mark most of the schema in activities.json to be optionally null, since it wasn't clear through the api documentation which ones were nullable. I also ran ./gradlew format and pushed all the formatting changes, so I think everything is good to go!

@marcosmarxm marcosmarxm merged commit 1b1b52d into airbytehq:master Oct 26, 2021
@terencecho terencecho deleted the source-strava branch October 27, 2021 07:10
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
* Initial commit for new source: Strava

* Update readme and invalid_config

* Update PR number in docs

* Update docs/integrations/README

* Annotae spec with airbyte_secret

* Change starting_after input to start_date and add try/catch connection check

* Add connector to connector index

* Address PR commends and separate streams

* Set start_date as a required field

* Remove nexted objects from schema

* Remove schema TODO.md

* Fix schema

* Edit activites schema to allow for null values

* Fix formatting

Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>

* Fix formatting

* Update source_definitions.yaml

Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
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.

None yet

5 participants