-
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
🎉 New Source: Retently #6966
🎉 New Source: Retently #6966
Conversation
Thanks for your contribution! We're going to review this tomorrow max by Friday. Can you share some screenshots showing the integratinos tests are passing? |
Sure! Unit tests:
Integration tests:
|
Hi @subhash-kloudio we add your contribution to our sprint. Sorry to not review it yet, there are so many connectors from Hacktober. |
auth = TokenAuthenticator(token, auth_method=auth_method) | ||
return [ Customers(auth), Companies(auth), Reports(auth) ] | ||
|
||
class Stream(HttpStream): |
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.
class Stream(HttpStream): | |
class RetentlyStream(HttpStream): |
Is a common pattern, can you change it?
api_key = config["api_key"] | ||
if not api_key: | ||
return False, f"API key is missing" | ||
else: | ||
return True, None |
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.
It's better call a try/except block.
See
airbyte/airbyte-integrations/connectors/source-surveymonkey/source_surveymonkey/source.py
Lines 18 to 27 in c83cafc
def check_connection(self, logger: AirbyteLogger, config: Mapping[str, Any]) -> Tuple[bool, Any]: | |
try: | |
authenticator = TokenAuthenticator(token=config["access_token"]) | |
start_date = pendulum.parse(config["start_date"]) | |
stream = Surveys(authenticator=authenticator, start_date=start_date) | |
records = stream.read_records(sync_mode=SyncMode.full_refresh) | |
next(records) | |
return True, None | |
except Exception as e: | |
return False, repr(e) |
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.
and api_key
is a required field dont need to check if is null.
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.
sorry for the delay in review! some small changes. I'll try to run tests locally to validate the code. Thanks @subhash-kloudio
No problem. Comments have been incorporated, thanks |
@@ -0,0 +1,15 @@ | |||
connector_image: airbyte/source-retently |
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.
connector_image: airbyte/source-retently | |
connector_image: airbyte/source-retently:dev |
"username": "foo", | ||
"password": "bar" |
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.
the config only use api_key
right?
ENTRYPOINT ["python", "/airbyte/integration_code/main.py"] | ||
|
||
LABEL io.airbyte.version=0.1.0 | ||
LABEL io.airbyte.name=airbyte/source_retently |
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.
LABEL io.airbyte.name=airbyte/source_retently | |
LABEL io.airbyte.name=airbyte/source-retently |
## [Quickrun](https://docs.airbyte.io/connector-development/tutorials/cdk-speedrun) | ||
|
||
1. `python -m venv .venv` | ||
2. `source .venv/bin/activate` | ||
3. `pip install -r requirements.txt` (Alter path in requirements.txt) | ||
4. `python main.py check --config sample_files/config.json` should report SUCCESS | ||
5. `python main.py check --config sample_files/invalid_config.json` should report FAILED | ||
6. `python main.py discover --config sample_files/config.json` presents schema | ||
7. `python main.py read --config sample_files/config.json --catalog sample_files/configured_catalog.json` runs source | ||
8. `docker build . -t <repo>/source-retently:<n>` builds docker image | ||
10. `docker push <repo>/source-retently:<n>` .. and push | ||
11. Add connector from http://localhost:8000/settings/source | ||
12. `pip install .[tests]` | ||
13. `python -m pytest unit_tests` | ||
14. `python -m pytest integration_tests` | ||
15. `python -m pytest integration_tests -p integration_tests.acceptance` |
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.
Please keep the README as same from other connectors.
@@ -0,0 +1,3 @@ | |||
{ | |||
"api_key": "b89fe9a7-d33c-43ab-a215-373c27593a69" |
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.
you dont need to let your api key here, change to only an example.
"id": { "type": "string" }, | ||
"createdDate": { "type": "string" }, | ||
"tags": { "type": "array", "items": { "type": "string" } }, | ||
"cxMetrics": { "type": "object" }, |
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.
cxMetrics is not possible to create the object or is dynamic?
"type": "object", | ||
"properties": { | ||
"last": { "type": "object" }, | ||
"trend": { "type": "array" }, |
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.
is not possible to add the nested objecT?
see API example
day" : "2020-01-12",
"promoters" : 292,
"passives" : 194,
"detractors" : 25,
"total" : 511,
"score" : 52
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.
Some comments, integration tests are working. After solving the comments we can merge this. Please make sure to resolve all problems when running ./gradlew format
there is some libs imported and not used.
token = "" | ||
auth = TokenAuthenticator(token, auth_method=auth_method) |
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.
token = "" | |
auth = TokenAuthenticator(token, auth_method=auth_method) | |
auth = TokenAuthenticator(token="", auth_method=auth_method) |
@property | ||
@abstractmethod | ||
def json_path(self): | ||
pass |
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.
Why this function?
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.
It is overriden in the specific stream classes to provide the path within the response that holds the stream data. Eg. subscribers
for Customers
def test_check_connection(mocker): | ||
source = SourceRetently() | ||
logger_mock, config_mock = MagicMock(), MagicMock() | ||
assert source.check_connection(logger_mock, config_mock) == (True, None) |
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.
This is not working.
> assert source.check_connection(logger_mock, config_mock) == (True, None)
E assert (False,\n "HTTPError('401 Client Error: Unauthorized for url: "\n "https://app.retently.com/api/v2/companies')") == (True, None)
E At index 0 diff: False != True
E Full diff:
E (
E - True,
E - None,
E + False,
E + "HTTPError('401 Client Error: Unauthorized for url: "
E + "https://app.retently.com/api/v2/companies')",
E )
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.
Fixed
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.
Fixed
Thanks.
|
You need to run |
Done! And reported problems fixed! Please let me know if there's anything else @marcosmarxm |
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.
LGTM! Great work @subhash-kloudio
* retently * PR comments * PR comments * mask api keys * README * keep gradle happy
* retently * PR comments * PR comments * mask api keys * README * keep gradle happy
What
How
Recommended reading order
x.java
y.python
Pre-merge Checklist
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md