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 SurveyMonkey: fix add missing params to stream survey_responses #25109

Merged

Conversation

leo-schick
Copy link
Contributor

What

Incomplete incremental sync. of stream survey_responses. See issue #25108

How

Provide the additional params sort_by, sort_order and per_page (to reduce the number of API calls)

🚨 User Impact 🚨

I do not expect a crash or something since this is not a breaking change, but since the historical data is incomplete I strongly recommend everybody who has an active connection to SurveyMonek to perform once a full sync after upgrading to a version with this fix.

Pre-merge Checklist

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

  • Connector version has been incremented

  • Documentation updated

    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. 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
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

@leo-schick
Copy link
Contributor Author

leo-schick commented Apr 12, 2023

Note: I did not test this change.

@marcosmarxm
Copy link
Member

marcosmarxm commented Apr 13, 2023

/test connector=connectors/source-surveymonkey

🕑 connectors/source-surveymonkey https://github.com/airbytehq/airbyte/actions/runs/4688636691
❌ connectors/source-surveymonkey https://github.com/airbytehq/airbyte/actions/runs/4688636691
🐛 https://gradle.com/s/eyyqgpdkgtbtq

Build Failed

Test summary info:

	 =========================== short test summary info ============================
	 FAILED unit_tests/test_streams.py::test_surveys_responses_request_params[stream_state0-params0]
	 FAILED unit_tests/test_streams.py::test_surveys_responses_request_params[stream_state1-params1]
	 �[31m================== �[31m�[1m2 failed�[0m, �[32m27 passed�[0m, �[33m74 warnings�[0m�[31m in 4.80s�[0m�[31m ===================�[0m

@marcosmarxm
Copy link
Member

@leo-schick please check unit test failures. These tests you can correct and submit a fix. After I'll run tests again.

@leo-schick
Copy link
Contributor Author

@marcosmarxm Please rerun ...

@leo-schick
Copy link
Contributor Author

@marcosmarxm give it another try....

@marcosmarxm
Copy link
Member

marcosmarxm commented Apr 13, 2023

/test connector=connectors/source-surveymonkey

🕑 connectors/source-surveymonkey https://github.com/airbytehq/airbyte/actions/runs/4689159015
❌ connectors/source-surveymonkey https://github.com/airbytehq/airbyte/actions/runs/4689159015
🐛 https://gradle.com/s/gedq2a7j6tzwu

Build Failed

Test summary info:

Could not find result summary

@leo-schick leo-schick force-pushed the fix-Source-SurveyMonkey-params branch from 676caec to 454fd14 Compare April 13, 2023 12:51
@leo-schick
Copy link
Contributor Author

@marcosmarxm please try once more...

I am actually confused about the last error we got... if it does not succeed, I think I will have to run the integration test locally on my production account ...

@marcosmarxm
Copy link
Member

marcosmarxm commented Apr 13, 2023

/test connector=connectors/source-surveymonkey

🕑 connectors/source-surveymonkey https://github.com/airbytehq/airbyte/actions/runs/4690837508
❌ connectors/source-surveymonkey https://github.com/airbytehq/airbyte/actions/runs/4690837508
🐛 https://gradle.com/s/3k2wacmdb4b5o

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs0] - AssertionError: as...
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
============= 1 failed, 38 passed, 1 skipped in 251.04s (0:04:11) ==============

@leo-schick
Copy link
Contributor Author

@marcosmarxm to be honest, I have no idea why this is happening...

@marcosmarxm
Copy link
Member

@leo-schick the error is in our side. I'm working to fix it.

@marcosmarxm
Copy link
Member

marcosmarxm commented Apr 17, 2023

/test connector=connectors/source-surveymonkey

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

Name                              Stmts   Miss  Cover
-----------------------------------------------------
source_surveymonkey/__init__.py       2      0   100%
source_surveymonkey/streams.py      140      9    94%
source_surveymonkey/source.py        65     13    80%
-----------------------------------------------------
TOTAL                               207     22    89%

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:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:578: The previous and actual discovered catalogs are identical.
================== 39 passed, 2 skipped in 259.42s (0:04:19) ===================

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 17, 2023
@marcosmarxm
Copy link
Member

marcosmarxm commented Apr 27, 2023

/publish connector=connectors/source-surveymonkey

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


Connector Version Did it publish? Were definitions generated?
connectors/source-surveymonkey 0.2.1

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

@marcosmarxm marcosmarxm merged commit f4d2c64 into airbytehq:master Apr 27, 2023
34 of 44 checks passed
@leo-schick leo-schick deleted the fix-Source-SurveyMonkey-params branch April 27, 2023 16:31
marcosmarxm added a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
…ses (airbytehq#25109)

* add missing params to stream survey_responses

* fix unit test test_surveys_responses_request_params

* use next_page_token when given in survey_responses stream

* fix integration tests

* fix build

* update expected records

* update expected records complete

* fix expected records

* bump connector version and update docs

* Update source_definitions.yaml

* auto-bump connector version

---------

Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants