-
Notifications
You must be signed in to change notification settings - Fork 4k
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: Kyriba #12748
🎉 New Source: Kyriba #12748
Conversation
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.
a few comments! but loved your implementation @wjwatkinson !!!
[[source]] | ||
url = "https://pypi.org/simple" | ||
verify_ssl = true | ||
name = "pypi" | ||
|
||
[packages] | ||
source-acceptance-test = {editable = true, file = "file:///Users/willwatkinson/src/github.com/airbytehq/airbyte/airbyte-integrations/bases/source-acceptance-test"} | ||
source-kyriba = {editable = true, file = "file:///Users/willwatkinson/src/github.com/airbytehq/airbyte/airbyte-integrations/connectors/source-kyriba"} | ||
|
||
[dev-packages] | ||
|
||
[requires] | ||
python_version = "3.9" |
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.
can you remove this file @wjwatkinson ?
"streams": [ | ||
{ | ||
"name": "TODO fix this file", | ||
"supported_sync_modes": ["full_refresh", "incremental"], | ||
"source_defined_cursor": true, | ||
"default_cursor_field": "column1", | ||
"json_schema": { | ||
"$schema": "http://json-schema.org/draft-07/schema#", | ||
"type": "object", | ||
"properties": { | ||
"column1": { |
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 remove this file.
"currency": { | ||
"type": "object", | ||
"properties": { | ||
"code": { | ||
"type": "string", | ||
"example": "CODE_ID", | ||
"description": "Code identifier." | ||
}, | ||
"uuid": { | ||
"type": "string", | ||
"format": "uuid", | ||
"example": "123e4567-e89b-12d3-a456-426655440001", | ||
"description": "UUID identifier. Has priority over the code" | ||
} | ||
}, | ||
"title": "ReferenceModel", | ||
"description": "Represents possible identifiers for resource. Should be provided at least one identifier (code or uuid). In the case of providing uuid and code, uuid will be used for resolving the reference." | ||
}, | ||
"value": { |
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.
what this schema about?
end_date: str = None, | ||
): | ||
self.gateway_url = gateway_url | ||
self.version = 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.
can you rename it to api_version
?
return {**data, **nested} | ||
|
||
def parse_response(self, response: requests.Response, **kwargs) -> Iterable[Mapping]: | ||
return iter(response.json().get("results")) |
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.
usually connectors use yield from response.json().get("results")
"examples": ["demo.kyriba.com"], | ||
"pattern":"^[a-zA-Z0-9._-]*\\.[a-zA-Z0-9._-]*\\.[a-z]*" | ||
}, | ||
"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.
api_version
}, | ||
"end_date": { | ||
"type": "string", | ||
"description": "The date the sync should end.", |
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.
If set to empty will sync data to most recent date. (maybe adding this to become clear to users)
self.start_date = start_date or date.isoformat(date.today()) | ||
self.end_date = date.fromisoformat(end_date) if end_date else 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.
why for end date you convert fromisoformat
and didn't apply for start date?
account_uuids = self.get_account_uuids() | ||
# we can query a max of 31 days at a time | ||
days_inc = 31 | ||
start_date = date.fromisoformat(self.start_date) |
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're doing the isoformat here, maybe apply when creating the object.
# we can query a max of 31 days at a time | ||
days_inc = 31 | ||
start_date = date.fromisoformat(self.start_date) | ||
end_date = self.end_date or date.today() |
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.
what happens if you don't inform start and end date? will get any data at all?
|
||
class Accounts(KyribaStream): | ||
primary_key = "uuid" | ||
|
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 enable cache to not run full refresh two times.
@marcosmarxm Thanks for the review. This is the first source I have contributed and the Kyriba API is not at all standard, so the second set of eyes is really helpful. |
Sure! |
b04d2b8
to
0c0f1ed
Compare
@marcosmarxm all your comments should have been addressed with updates. Let me know if I missed anything, or you see further room for improvement. |
@wjwatkinson do you mind sharing the integration test output again? |
and solve the conflict with SUMMARY.md looks this file was deleted |
88e4811
to
3d493f6
Compare
Integration Tests
|
@wjwatkinson do you think is ready to be merged? Can you solve conflict with |
f6b26b4
to
b227d4b
Compare
Unit Tests
Integration Tests
|
@wjwatkinson can you add the source in the seed file? Check step 4 of https://docs.airbyte.com/connector-development/#4-publish-the-connector |
/publish connector=connectors/source-kyriba run-tests=false
|
The command to publish without integration account was removed. I'll work this week to implement it back and publish your contribution. |
/publish connector=connectors/source-kyriba run-tests=false
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-kyriba run-tests=false
if you have connectors that successfully published but failed definition generation, follow step 4 here |
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 @wjwatkinson
/test connector=connectors/source-kyriba
Build FailedTest summary info:
|
@grubberr this was merged as alpha connector without credentials. |
What
Add a new source for Kyriba
How
Use the python cdk
🚨 User Impact 🚨
None
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/SUMMARY.md
docs/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 hereTests
Unit
pipenv run pytest -s unit_tests/ Test session starts (platform: darwin, Python 3.9.12, pytest 6.2.5, pytest-sugar 0.9.4) cachedir: .pytest_cache rootdir: /Users/willwatkinson/src/github.com/airbytehq/airbyte, configfile: pytest.ini plugins: sugar-0.9.4, mock-3.7.0, timeout-1.4.2 collecting ... airbyte-integrations/connectors/source-kyriba/unit_tests/test_account_sub_stream.py::test_get_account_uuids ✓ 3% ▍ airbyte-integrations/connectors/source-kyriba/unit_tests/test_account_sub_stream.py::test_parse_response ✓ 6% ▋ airbyte-integrations/connectors/source-kyriba/unit_tests/test_bank_balances_stream.py::test_stream_slices ✓ 9% ▉ airbyte-integrations/connectors/source-kyriba/unit_tests/test_bank_balances_stream.py::test_path ✓ 11% █▎ airbyte-integrations/connectors/source-kyriba/unit_tests/test_bank_balances_stream.py::test_request_params ✓ 14% █▌ airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_balances_stream.py::test_stream_slices ✓ 17% █▊ airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_balances_stream.py::test_path ✓ 20% ██ airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_balances_stream.py::test_request_params ✓ 23% ██▍ airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_flows.py::test_path ✓ 26% ██▋ airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_flows.py::test_stream_slices_new ✓ 29% ██▉ airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_flows.py::test_stream_slices_state ✓ 31% ███▎ airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_flows.py::test_all_request_params ✓ 34% ███▌ airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_flows.py::test_parse_response ✓ 37% ███▊ airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py::test_cursor_field ✓ 40% ████ airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py::test_get_updated_state ✓ 43% ████▍ airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py::test_stream_slices ✓ 46% ████▋ airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py::test_supports_incremental ✓ 49% ████▉ airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py::test_source_defined_cursor ✓ 51% █████▎ airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py::test_stream_checkpoint_interval ✓ 54% █████▌ airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py::test_all_request_params ✓ 57% █████▊ airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py::test_min_request_params ✓ 60% ██████ airbyte-integrations/connectors/source-kyriba/unit_tests/test_source.py::test_check_connection ✓ 63% ██████▍ airbyte-integrations/connectors/source-kyriba/unit_tests/test_source.py::test_streams ✓ 66% ██████▋ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_request_params ✓ 69% ██████▉ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_next_page_token ✓ 71% ███████▎ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_parse_response ✓ 74% ███████▌ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_request_headers ✓ 77% ███████▊ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_http_method ✓ 80% ████████ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_should_retry[HTTPStatus.OK-False] ✓ 83% ████████▍ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_should_retry[HTTPStatus.BAD_REQUEST-False] ✓ 86% ████████▋ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_should_retry[HTTPStatus.TOO_MANY_REQUESTS-True] ✓ 89% ████████▉ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_should_retry[HTTPStatus.INTERNAL_SERVER_ERROR-True] ✓91% █████████▎ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_should_retry_401 ✓ 94% █████████▌ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_backoff_time ✓ 97% █████████▊ airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py::test_unnest ✓ 100% ██████████ =============================================================== warnings summary ================================================================ ../../../../../../../.local/share/virtualenvs/source-kyriba-2g9Yv4kW/lib/python3.9/site-packages/airbyte_cdk/sources/utils/transform.py:12 /Users/willwatkinson/.local/share/virtualenvs/source-kyriba-2g9Yv4kW/lib/python3.9/site-packages/airbyte_cdk/sources/utils/transform.py:12: DeprecationWarning: Call to deprecated class AirbyteLogger. (Use logging.getLogger('airbyte') instead) -- Deprecated since version 0.1.47. logger = AirbyteLogger()../../../../../../../.local/share/virtualenvs/source-kyriba-2g9Yv4kW/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/rate_limiting.py:19
/Users/willwatkinson/.local/share/virtualenvs/source-kyriba-2g9Yv4kW/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/rate_limiting.py:19: DeprecationWarning: Call to deprecated class AirbyteLogger. (Use logging.getLogger('airbyte') instead) -- Deprecated since version 0.1.47.
logger = AirbyteLogger()
../../../../../../../.local/share/virtualenvs/source-kyriba-2g9Yv4kW/lib/python3.9/site-packages/airbyte_cdk/utils/event_timing.py:13
/Users/willwatkinson/.local/share/virtualenvs/source-kyriba-2g9Yv4kW/lib/python3.9/site-packages/airbyte_cdk/utils/event_timing.py:13: DeprecationWarning: Call to deprecated class AirbyteLogger. (Use logging.getLogger('airbyte') instead) -- Deprecated since version 0.1.47.
logger = AirbyteLogger()
airbyte-integrations/connectors/source-kyriba/unit_tests/test_account_sub_stream.py: 4 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_bank_balances_stream.py: 6 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_balances_stream.py: 6 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_flows.py: 5 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py: 8 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_source.py: 10 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py: 12 warnings
/Users/willwatkinson/.local/share/virtualenvs/source-kyriba-2g9Yv4kW/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()
airbyte-integrations/connectors/source-kyriba/unit_tests/test_account_sub_stream.py: 4 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_bank_balances_stream.py: 6 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_balances_stream.py: 6 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_cash_flows.py: 5 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_incremental_streams.py: 8 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_source.py: 10 warnings
airbyte-integrations/connectors/source-kyriba/unit_tests/test_streams.py: 12 warnings
/Users/willwatkinson/.local/share/virtualenvs/source-kyriba-2g9Yv4kW/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.20s):
35 passed
Integration
➜ docker build . -t airbyte/source-kyriba:dev && pipenv run python -m pytest -p integration_tests.acceptance [+] Building 0.8s (17/17) FINISHED => [internal] load build definition from Dockerfile 0.0s => => transferring dockerfile: 37B 0.0s => [internal] load .dockerignore 0.0s => => transferring context: 34B 0.0s => [internal] load metadata for docker.io/library/python:3.7.11-alpine3.14 0.7s => [auth] library/python:pull token for registry-1.docker.io 0.0s => [internal] load build context 0.0s => => transferring context: 12.43kB 0.0s => [base 1/1] FROM docker.io/library/python:3.7.11-alpine3.14@sha256:fcf44c5aaf990b16a37f5ad9f27d98c5da8b9bf280e48fe3383820882926ddee 0.0s => CACHED [builder 1/4] WORKDIR /airbyte/integration_code 0.0s => CACHED [builder 2/4] RUN apk --no-cache upgrade && pip install --upgrade pip && apk --no-cache add tzdata build-base 0.0s => CACHED [builder 3/4] COPY setup.py ./ 0.0s => CACHED [builder 4/4] RUN pip install --prefix=/install . 0.0s => CACHED [stage-2 2/7] COPY --from=builder /install /usr/local 0.0s => CACHED [stage-2 3/7] COPY --from=builder /usr/share/zoneinfo/Etc/UTC /etc/localtime 0.0s => CACHED [stage-2 4/7] RUN echo "Etc/UTC" > /etc/timezone 0.0s => CACHED [stage-2 5/7] RUN apk --no-cache add bash 0.0s => CACHED [stage-2 6/7] COPY main.py ./ 0.0s => [stage-2 7/7] COPY source_kyriba ./source_kyriba 0.0s => exporting to image 0.0s => => exporting layers 0.0s => => writing image sha256:5e9d6e4111866837340d92f7a78c7f5ba7849fdba9736def1dc5274b3b51c998 0.0s => => naming to docker.io/airbyte/source-kyriba:dev 0.0sUse 'docker scan' to run Snyk tests against images to find vulnerabilities and learn how to fix them
Test session starts (platform: darwin, Python 3.9.12, pytest 6.2.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /Users/willwatkinson/src/github.com/airbytehq/airbyte, configfile: pytest.ini
plugins: sugar-0.9.4, mock-3.7.0, timeout-1.4.2
collecting ...
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_config_match_spec[inputs0] ✓5% ▌
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_match_expected[inputs0] ✓9% ▉
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_docker_env[inputs0] ✓ 14% █▍
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_oneof_usage[inputs0] ✓ 18% █▊
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_required[inputs0] ✓ 23% ██▍
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_optional[inputs0] ✓ 27% ██▊
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_has_secret[inputs0] ✓ 32% ███▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_secret_never_in_the_output[inputs0] ✓36% ███▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_defined_refs_exist_in_json_spec_file[inputs0] ✓41% ████▏
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestSpec.test_oauth_flow_parameters[inputs0] ✓45% ████▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs0] ✓ 50% █████
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestConnection.test_check[inputs1] ✓ 55% █████▌
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_discover[inputs0] ✓59% █████▉
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_defined_cursors_exist_in_schema[inputs0] ✓64% ██████▍
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_defined_refs_exist_in_schema[inputs0] ✓68% ██████▊
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_defined_keyword_exist_in_schema[inputs0-allOf] ✓73% ███████▍
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_defined_keyword_exist_in_schema[inputs0-not] ✓77% ███████▊
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestDiscovery.test_primary_keys_exist_in_schema[inputs0] ✓82% ████████▎
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py::TestBasicRead.test_read[inputs0] ✓ 86% ████████▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_full_refresh.py::TestFullRefresh.test_sequential_reads[inputs0] ✓91% █████████▏
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py::TestIncremental.test_two_sequential_reads[inputs0] ✓95% █████████▋
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py::TestIncremental.test_state_with_abnormally_large_values[inputs0] ✓100% ██████████
{"type": "LOG", "log": {"level": "INFO", "message": "/Users/willwatkinson/src/github.com/airbytehq/airbyte/airbyte-integrations/connectors/source-kyriba - SAT run - 82a441ccfc7391d6e1eb4b5b2a342af78eff4146 - PASSED"}}
=============================================================== warnings summary ================================================================
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py: 16 warnings
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_full_refresh.py: 1 warning
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py: 2 warnings
/Users/willwatkinson/.local/share/virtualenvs/source-kyriba-2g9Yv4kW/lib/python3.9/site-packages/docker/utils/utils.py:52: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
s1 = StrictVersion(v1)
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py: 16 warnings
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_full_refresh.py: 1 warning
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_incremental.py: 2 warnings
/Users/willwatkinson/.local/share/virtualenvs/source-kyriba-2g9Yv4kW/lib/python3.9/site-packages/docker/utils/utils.py:53: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
s2 = StrictVersion(v2)
-- Docs: https://docs.pytest.org/en/stable/warnings.html
Results (245.97s):
22 passed