-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 Retently - Add Feedback, NPS Score, Campaigns streams #19381
Source Retently - Add Feedback, NPS Score, Campaigns streams #19381
Conversation
Hello 👋, first thank you for this amazing contribution. We really appreciate the effort you've made to improve the project. If you have any questions feel free to send me a message in Slack! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @schlattk, I am sorry for the delay!
Thank you again for your contribution.
I made a couple of comments on your PR, feel free to come back to and/or challenge me if something is unclear!
Also, could you post a screenshot of the acceptance tests' output after your changes?
if stream_data: | ||
for d in stream_data: | ||
yield d | ||
data = response.json().get("data") or response.json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to be explicit depending on the stream, and therefore surcharge the parse_response
when the expected response differs.
It should make potential errors easier to spot!
yield d | ||
data = response.json().get("data") or response.json() | ||
stream_data = data.get(self.json_path) if self.json_path else data | ||
if type(stream_data) == list : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above: since it seems that this is for only one specific stream (nps
), I think it would be better to override the parse_response
method of that specific stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @cptjacky
do you mean to do it in the parse_response method something like if self.name == nps yield x else yield y
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @schlattk, I meant defining a different parse_response
method on the NpsStream class itself (no if
required in that situation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cptjacky do you have an example of another source where it is done in this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schlattk https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-intercom/source_intercom/source.py#L342, the main class has a default parse_response
, and then other streams define another parse_response
with a different logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok @cptjacky I have implemented this now but some of the acceptance tests are failing not sure why as it doesn't seem to relate to changes I made. Would be interesting to see whether they also fail with the Airbyte test account credentials. Happy to run it if you can share the credentials
Results (333.83s): 23 passed 6 failed - airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:109 TestSpec.test_match_expected[inputs0] - airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:121 TestSpec.test_enum_usage[inputs0] - airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:135 TestSpec.test_oneof_usage[inputs0] - airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:313 TestSpec.test_additional_properties_is_true[inputs0] - airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:330 TestConnection.test_check[inputs2] - airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:638 TestBasicRead.test_read[inputs0]
Hey @schlattk, for some of these errors (the ones I can run without credentials), it seems to be because new versions of the acceptance-tests are more rigid about deprecated keys. In our specific case, I think we need to take a look at the diff --git a/airbyte-integrations/connectors/source-retently/source_retently/spec.json b/airbyte-integrations/connectors/source-retently/source_retently/spec.json
index 56042fff6d..59b7f34080 100644
--- a/airbyte-integrations/connectors/source-retently/source_retently/spec.json
+++ b/airbyte-integrations/connectors/source-retently/source_retently/spec.json
@@ -20,8 +20,6 @@
"auth_type": {
"type": "string",
"const": "Client",
- "enum": ["Client"],
- "default": "Client",
"order": 0
},
"client_id": {
@@ -52,8 +50,6 @@
"auth_type": {
"type": "string",
"const": "Token",
- "enum": ["Token"],
- "default": "Token",
"order": 0
},
"api_key": { This diff should fix at least 2 or 3 errors. Can you try it and send me a screenshot of the acceptance tests? If there is still errors, could you also paste the errors here? Thank you! |
@cptjacky errors are still the same - any chance we can run this with test credentials? |
/test connector=connectors/source-retently
|
/test connector=connectors/source-retently
|
@schlattk can you rebase on master? The CI was breaking before and we fixed it on master but it's not being reflected on your branch. |
/test connector=connectors/source-retently
Build FailedTest summary info:
|
Hey @schlattk, the two fails here should be fixed by the diff I proposed earlier, can you apply it to this PR? |
Hi @cptjacky, I have made those changes - can you run tests again please |
/test connector=connectors/source-retently
Build FailedTest summary info:
|
Hi @cptjacky We have still two failing tests not sure I understand why. Are they passing if you run them on the current version? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @schlattk, I added a comment I think is responsible for one fail. You should also re-set the additionalProperties
to true
like it was before your last commit I think
"additionalQuestions": [], | ||
"feedbackTags": [], | ||
"notes": [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these fields in the schema are not valid. Their definition should be an object, not an empty list
@schlattk just bumping this, are you able to address the comments above? We're trying to get this merged before the end of the week. Otherwise it might get even more delayed because of the upcoming holidays. We're fairly close to finishing the review here. Let me know what I can do to help. |
@sajarin changes made |
/test connector=connectors/source-retently
Build FailedTest summary info:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @schlattk, please see my latest comments on the schema
airbyte-integrations/connectors/source-retently/source_retently/schemas/feedback.json
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-retently/source_retently/schemas/feedback.json
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-retently/source_retently/schemas/feedback.json
Outdated
Show resolved
Hide resolved
/test connector=connectors/source-retently
Build PassedTest summary info:
|
/publish connector=connectors/source-retently
if you have connectors that successfully published but failed definition generation, follow step 4 here |
@cptjacky ok to level 0.1.3? |
Should be good to publish and merge soon, just waiting on an issue on master to clear up before triggering the CI again |
/publish connector=connectors/source-retently
if you have connectors that successfully published but failed definition generation, follow step 4 here |
Hey @schlattk the CI failed to generate definitions because it couldn't commit to your branch. Could you bump the source-retently version in Let me know if you run into any issues |
I'm having some issues running gradle command |
what's the issue? Can you post logs/screenshots? |
@sajarin I'm on mac - getting this although jdk is installed
|
What
Added streams for source retently: feedback, nps score, campaigns
fixed failing connection check on Companies stream
How
added streams to source.py as well as new schemas
changed check_connection to use customers stream
Recommended reading order
x.java
y.python
🚨 User Impact 🚨
User has access to more streams, check_connection fails when companies stream is empty so changed it to use customer stream which will always have data.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Put your unit tests output here.
Test session starts (platform: darwin, Python 3.9.7, pytest 6.2.5, pytest-sugar 0.9.6)
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/Users/konrad/retently-fork/airbyte/airbyte-integrations/connectors/source-retently/.hypothesis/examples')
rootdir: /Users/konrad/retently-fork/airbyte, configfile: pytest.ini
plugins: hypothesis-6.54.6, requests-mock-1.9.3, mock-3.6.1, sugar-0.9.6, timeout-1.4.2, cov-3.0.0
collecting ...
airbyte-integrations/connectors/source-retently/unit_tests/test_source.py::test_check_connection ✓ 8% ▉
airbyte-integrations/connectors/source-retently/unit_tests/test_source.py::test_streams ✓ 17% █▋
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_request_params ✓ 25% ██▌
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_next_page_token ✓ 33% ███▍
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_parse_response ✓ 42% ████▎
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_request_headers ✓ 50% █████
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_http_method ✓ 58% █████▉
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_should_retry[HTTPStatus.OK-False] ✓ 67% ██████▋
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_should_retry[HTTPStatus.BAD_REQUEST-False] ✓ 75% ███████▌
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_should_retry[HTTPStatus.TOO_MANY_REQUESTS-True] ✓ 83% ████████▍
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_should_retry[HTTPStatus.INTERNAL_SERVER_ERROR-True] ✓ 92% █████████▎
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py::test_backoff_time ✓ 100% ██████████
================================================================================== warnings summary ===================================================================================
airbyte-integrations/connectors/source-retently/unit_tests/test_source.py::test_check_connection
airbyte-integrations/connectors/source-retently/unit_tests/test_source.py::test_streams
/Users/konrad/retently-fork/airbyte/airbyte-integrations/connectors/source-retently/source_retently/source.py:33: DeprecationWarning: Call to deprecated class TokenAuthenticator. (Use airbyte_cdk.sources.streams.http.requests_native_auth.TokenAuthenticator instead) -- Deprecated since version 0.1.20.
return TokenAuthenticator(token="", auth_method=auth_method)
airbyte-integrations/connectors/source-retently/unit_tests/test_source.py: 9 warnings
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py: 10 warnings
/Users/konrad/retently-fork/airbyte/airbyte-integrations/connectors/source-retently/.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)
airbyte-integrations/connectors/source-retently/unit_tests/test_source.py: 7 warnings
airbyte-integrations/connectors/source-retently/unit_tests/test_streams.py: 10 warnings
/Users/konrad/retently-fork/airbyte/airbyte-integrations/connectors/source-retently/.venv/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/http.py:43: DeprecationWarning: Call to deprecated class NoAuth. (Set
authenticator=None
instead) -- Deprecated since version 0.1.20.self._authenticator: HttpAuthenticator = NoAuth()
-- Docs: https://docs.pytest.org/en/stable/warnings.html
Results (0.77s):
12 passed
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.