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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃帀 New Source: Hubplanner #12145

Closed
wants to merge 7 commits into from

Conversation

RickRen7575
Copy link
Contributor

@RickRen7575 RickRen7575 commented Apr 19, 2022

What

I added a source connector for the Hubplanner API. Here is the information on the Hubplanner API: https://github.com/hubplanner/API

How

I utilized the generate.sh script to provide a scaffold and then followed the YouTube video here: https://www.youtube.com/watch?v=kJ3hLoNfz_E&list=PLgyvStszwUHjb-t_wfGizz1VZgwX7Td5b&index=2

I also took inspiration from the Singer tap for Hubplanner: https://github.com/rangle/tap-hubplanner

Recommended reading order

Anything in the source-hubplanner directory should be fair game to start at.

馃毃 User Impact 馃毃

No breaking changes.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Community member

  • 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

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 by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here
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
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. 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
Connector Generator
  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

I tested running check, discover, and read commands with the configured_catalog.json file provided, and it worked. I also tested creating the source in the UI and creating a connection that loads Hubplanner data to BigQuery and I also tested loading to a local JSON file. Both worked!

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/connectors Connector related issues label Apr 19, 2022
@RickRen7575
Copy link
Contributor Author

I still need to add integration and unit tests, but I just wanted to open the PR first

@RickRen7575 RickRen7575 changed the title Hubplanner Source Connector New Source Connector: Hubplanner using CDK Apr 19, 2022
@RickRen7575
Copy link
Contributor Author

@marcosmarxm what is a good example of unit/integration test suite comparable to Hubplanner? Looks like some connectors implement tests and others don't, so just want to know what is best to compare against and structure mine

def test_check_connection(mocker):
source = SourceHubplanner()
logger_mock, config_mock = MagicMock(), MagicMock()
assert source.check_connection(logger_mock, config_mock) == (True, None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcosmarxm for example, I am a little confused how I am supposed to set this up to pass. All the other connectors look like they are setting up fixtures that just have like "api_key": "test_api_key" as their config. I don't understand why something like that would pass without a valid config.

Copy link
Contributor

Choose a reason for hiding this comment

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

The goal of this test is to assert the behavior of check_connection on successful or failed connection response by the API. You should use request_mock to forge mock success and failure request and assert on the behavior of check_connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alafanechere so I would copy basically an example of a "failed response" from the hubplanner API, set up a test to mock that as the response, and make sure that check_connection fails in that instance?

@alafanechere alafanechere self-assigned this Apr 22, 2022
@alafanechere
Copy link
Contributor

Hi @RickRen7575 thank you for this contribution. We need to create a sandbox Hubplanner account to run tests on your connectors. I'll go for a review once we have this account.

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Could you please:

  • Format your code with ./gradlew format
  • Run the acceptance tests and share their output:
    ./gradlew :airbyte-integrations:connectors:source-hubplanner:integrationTest (this is a built-in test suite that is configured in acceptance-test-config.yml)

@RickRen7575
Copy link
Contributor Author

That would be awesome!!

@alafanechere
Copy link
Contributor

  • Format your code with ./gradlew format
  • Run the acceptance tests and share their output:
    ./gradlew :airbyte-integrations:connectors:source-hubplanner:integrationTest (this is a built-in test suite that is configured in acceptance-test-config.yml)

@RickRen7575 are you ready to perform these tasks? I won't be able to do this myself as I've no maintainer permission on your organization's fork.

@alafanechere
Copy link
Contributor

Hey @RickRen7575,
I'm closing this PR because I did not receive updates from your side and I don't have permission to push the changes I requested. Feel free to re-open whenever you like, we now have a sandbox account for Hubplanner to test your connector and make this PR move forward.

@RickRen7575
Copy link
Contributor Author

@alafanechere sorry for the delay - I've been out for the last 3 weeks. I'll run these things and then push updates!

@RickRen7575
Copy link
Contributor Author

@alafanechere I don't see the ability to reopen this PR. Do you mind doing that on your end?

@alafanechere alafanechere reopened this May 16, 2022
@alafanechere
Copy link
Contributor

@RickRen7575 I reopened it!

@RickRen7575
Copy link
Contributor Author

@alafanechere I just tried running the integration tests and got this output:
`

Task :airbyte-integrations:bases:source-acceptance-test:installLocalReqs
[python] cmd /c .venv\Scripts\python.exe -m pip install .[dev,tests]
Processing c:\users\rickyrenner\documents\repos\airbyte\airbyte\airbyte-integrations\bases\source-acceptance-test
Preparing metadata (setup.py): started
Preparing metadata (setup.py): finished with status 'done'
WARNING: source-acceptance-test 0.0.0 does not provide the extra 'dev'
WARNING: source-acceptance-test 0.0.0 does not provide the extra 'tests'
Collecting airbyte-cdk~=0.1.56
Using cached airbyte_cdk-0.1.56-py3-none-any.whl (83 kB)
Collecting docker~=5.0.3
Using cached docker-5.0.3-py2.py3-none-any.whl (146 kB)
Collecting PyYAML~=5.4
Using cached PyYAML-5.4.1-cp310-cp310-win_amd64.whl
Collecting icdiff~=1.9
Using cached icdiff-1.9.1.tar.gz (9.1 kB)
Preparing metadata (setup.py): started
Preparing metadata (setup.py): finished with status 'done'
Collecting inflection~=0.5
Using cached inflection-0.5.1-py2.py3-none-any.whl (9.5 kB)
Collecting pdbpp~=0.10
Using cached pdbpp-0.10.3-py2.py3-none-any.whl (23 kB)
Collecting pydantic~=1.6
Using cached pydantic-1.9.0-cp310-cp310-win_amd64.whl (2.1 MB)
Requirement already satisfied: pytest~=6.1 in c:\users\rickyrenner\documents\repos\airbyte\airbyte\airbyte-integrations\bases\source-acceptance-test.venv\lib\site-packages (from source-acceptance-test==0.0.0) (6.1.2)
Collecting pytest-sugar~=0.9
Using cached pytest-sugar-0.9.4.tar.gz (12 kB)
Preparing metadata (setup.py): started
Preparing metadata (setup.py): finished with status 'done'
Collecting pytest-timeout~=1.4
Using cached pytest_timeout-1.4.2-py2.py3-none-any.whl (10 kB)
Collecting pprintpp~=0.4
Using cached pprintpp-0.4.0-py2.py3-none-any.whl (16 kB)
Collecting dpath~=2.0.1
Using cached dpath-2.0.6-py3-none-any.whl (15 kB)
Collecting jsonschema~=3.2.0
Using cached jsonschema-3.2.0-py2.py3-none-any.whl (56 kB)
Collecting jsonref==0.2
Using cached jsonref-0.2-py3-none-any.whl (9.3 kB)
Collecting pendulum
Using cached pendulum-2.1.2-cp310-cp310-win_amd64.whl
Collecting requests
Using cached requests-2.27.1-py2.py3-none-any.whl (63 kB)
Collecting vcrpy
Using cached vcrpy-4.1.1-py2.py3-none-any.whl (40 kB)
Collecting sentry-sdk~=1.5.1
Using cached sentry_sdk-1.5.12-py2.py3-none-any.whl (145 kB)
Collecting backoff
Using cached backoff-2.0.1-py3-none-any.whl (14 kB)
Collecting Deprecated~=1.2
Using cached Deprecated-1.2.13-py2.py3-none-any.whl (9.6 kB)
Collecting websocket-client>=0.32.0
Using cached websocket_client-1.3.2-py3-none-any.whl (54 kB)
INFO: pip is looking at multiple versions of to determine which version is compatible with other requirements. This could take a while.
INFO: pip is looking at multiple versions of airbyte-cdk to determine which version is compatible with other requirements. This could take a while.
INFO: pip is looking at multiple versions of jsonref to determine which version is compatible with other requirements. This could take a while.
INFO: pip is looking at multiple versions of source-acceptance-test[dev,tests] to determine which version is compatible with other requirements. This could take a while.
ERROR: Could not find a version that satisfies the requirement pywin32==227; sys_platform == "win32" (from docker) (from versions: 302, 303, 304)
ERROR: No matching distribution found for pywin32==227; sys_platform == "win32"
WARNING: You are using pip version 21.3.1; however, version 22.1 is available.
You should consider upgrading via the 'C:\Users\RickyRenner\Documents\Repos\airbyte\airbyte\airbyte-integrations\bases\source-acceptance-test.venv\Scripts\python.exe -m pip install --upgrade pip' command.

Task :airbyte-integrations:bases:source-acceptance-test:installLocalReqs FAILED`

@alafanechere
Copy link
Contributor

Are you using WSL to run this? Could you check the alternative pytest commands in the generated README file to run the acceptance tests?

@RickRen7575
Copy link
Contributor Author

I was trying to use just straight up Windows, but I can try WSL instead!

@bleonard bleonard added the bounty label Jun 2, 2022
@alafanechere
Copy link
Contributor

@RickRen7575 do you manage to make the test pass with WSL?

@RickRen7575
Copy link
Contributor Author

Hi @alafanechere I'm sorry but I've just been swamped recently. I hope I can get back on this soon. I tried running the tests and ran into an issue and then got sidetracked.

@marcosmarxm marcosmarxm changed the title New Source Connector: Hubplanner using CDK 馃帀 New Source: Hubplanner Aug 4, 2022
@marcosmarxm marcosmarxm mentioned this pull request Aug 10, 2022
37 tasks
@marcosmarxm
Copy link
Member

Thanks for the contribution @RickRen7575 I merged the code in #15521

@RickRen7575
Copy link
Contributor Author

@marcosmarxm oh great!

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

7 participants