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

Add new marketing emails (with statistics) stream #5840

Merged

Conversation

t0hai
Copy link
Contributor

@t0hai t0hai commented Sep 3, 2021

What

Add new stream to Hubspot connector for Marketing Email API (with statistics)

How

Extend existing Hubspot connector by adding:

  • MarketingEmailStream class
  • marketing_emails.json schema
  • entry for marketing_emails stream in sample_files/configured_catalog*

Recommended reading order

  1. api.py
  2. marketing_emails.json
  3. sample_files/configured_catalog.json

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Stream in existing Hubspot Connector

Community member or Airbyter

  • Community member
  • Unit & integration tests added and passing.
    • integration test did not passed due to JSON validation errors but modified connector is running successfully on Airbyte instance
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • 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
  • Connector added to connector index like described here

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
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here

screenshot-airbyte risus xyz_8000-2021 09 10-17_04_19

@github-actions github-actions bot added the area/connectors Connector related issues label Sep 3, 2021
@marcosmarxm
Copy link
Member

thanks for the contribution @n0rritt, do you need any assistance here? After finishing the change could you change the PR from draft to ready to review?

@marcosmarxm marcosmarxm self-assigned this Sep 6, 2021
@t0hai
Copy link
Contributor Author

t0hai commented Sep 6, 2021

@marcosmarxm I no longer get timeout >= 360ms error when I use the demo API key and raise the timeout_seconds: 3600 in acceptance-test-config.yml
but still get an error when I run python -m pytest -p integration_tests.acceptance:

self = <source_acceptance_test.tests.test_core.TestBasicRead object at 0x7fc755d8a970>, connector_config = SecretDict(******)
configured_catalog = ConfiguredAirbyteCatalog(streams=[ConfiguredAirbyteStream(stream=AirbyteStream(name='campaigns', json_schema={'$schema...l_refresh'>, cursor_field=None, destination_sync_mode=<DestinationSyncMode.overwrite: 'overwrite'>, primary_key=None)])
inputs = BasicReadTestConfig(config_path='secrets/config.json', configured_catalog_path='sample_files/configured_catalog_without_workflows.json', empty_streams=set(), expect_records=None, validate_schema=True, timeout_seconds=3600)
expected_records = [], docker_runner = <source_acceptance_test.utils.connector_runner.ConnectorRunner object at 0x7fc755d8af40>
detailed_logger = <Logger detailed_logger acceptance_tests_logs/test_core.py::TestBasicRead::test_read[inputs0].txt (DEBUG)>
    def test_read(
        self,
        connector_config,
        configured_catalog,
        inputs: BasicReadTestConfig,
        expected_records: List[AirbyteMessage],
        docker_runner: ConnectorRunner,
        detailed_logger,
    ):
        output = docker_runner.call_read(connector_config, configured_catalog)
        records = [message.record for message in filter_output(output, Type.RECORD)]
        assert records, "At least one record should be read using provided catalog"
        if inputs.validate_schema:
>           self._validate_schema(records=records, configured_catalog=configured_catalog)
../../bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:216: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../bases/source-acceptance-test/source_acceptance_test/tests/test_core.py:154: in _validate_schema
    streams_errors = verify_records_schema(records, configured_catalog)
../../bases/source-acceptance-test/source_acceptance_test/utils/asserts.py:80: in verify_records_schema
    errors = list(validator.iter_errors(record.data))
/home/hai/conda/envs/source-hubspot/lib/python3.8/site-packages/jsonschema/validators.py:328: in iter_errors
    for error in errors:
/home/hai/conda/envs/source-hubspot/lib/python3.8/site-packages/jsonschema/_validators.py:282: in properties
    for error in validator.descend(
/home/hai/conda/envs/source-hubspot/lib/python3.8/site-packages/jsonschema/validators.py:344: in descend
    for error in self.iter_errors(instance, schema):
/home/hai/conda/envs/source-hubspot/lib/python3.8/site-packages/jsonschema/validators.py:328: in iter_errors
    for error in errors:
/home/hai/conda/envs/source-hubspot/lib/python3.8/site-packages/jsonschema/_validators.py:282: in properties
    for error in validator.descend(
/home/hai/conda/envs/source-hubspot/lib/python3.8/site-packages/jsonschema/validators.py:344: in descend
    for error in self.iter_errors(instance, schema):
/home/hai/conda/envs/source-hubspot/lib/python3.8/site-packages/jsonschema/validators.py:328: in iter_errors
    for error in errors:
/home/hai/conda/envs/source-hubspot/lib/python3.8/site-packages/jsonschema/_validators.py:208: in format
    validator.format_checker.check(instance, format)
../../bases/source-acceptance-test/source_acceptance_test/utils/asserts.py:56: in check
    if not self.check_datetime(instance):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
value = None
    @staticmethod
    def check_datetime(value: str) -> bool:
>       valid_format = timestamp_regex.match(value)
E       TypeError: expected string or bytes-like object
../../bases/source-acceptance-test/source_acceptance_test/utils/asserts.py:45: TypeError

it seems to have a problem with the validation of a datetime field, but the only timestamp field that I can see in schemas/campaigns.json is following
"lastUpdatedTime": { "type": ["null", "integer"] }
but this should be an integer, do you have any idea what could be causing this?

@t0hai t0hai force-pushed the n0rritt/add_marketing_emails_stream branch 2 times, most recently from a71affc to a4651e9 Compare September 8, 2021 09:20
@marcosmarxm
Copy link
Member

@n0rritt the problem was that acceptance tests was expecting a value and you send None. I think this was already corrected. Could you update with master and try again?

Tomorrow I'll review the new stream, but overall looks great! Could you post a picture showing that all tests are passing too (after correcting the problem of course)?

@t0hai t0hai marked this pull request as ready for review September 10, 2021 13:59
@t0hai
Copy link
Contributor Author

t0hai commented Sep 10, 2021

@marcosmarxm I've marked the PR ready for review, but I could not manage to get the integration test running succesfully

when I use our Snapchat API key we get a timeout even when we raise the timeout to 3600s for the test

when I use the demo API key I get a JSON validation error because it seems that the response from the demo API does not follow the current schema definition

I was however able to run the modified Hubspot connector on our Airbyte instance successfully by registering our own docker image on Docker Hub https://hub.docker.com/repository/docker/dat4plu5d3nt4l/source-hubspot

maybe the integration test will work for your test account better, but please tell me if I should adapt something

@midavadim
Copy link
Contributor

we should update docs and add change history

Copy link
Contributor

@midavadim midavadim left a comment

Choose a reason for hiding this comment

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

Please add change history to docs

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Sep 15, 2021
@t0hai t0hai force-pushed the n0rritt/add_marketing_emails_stream branch from 60df19b to bbedd67 Compare September 15, 2021 08:09
@t0hai
Copy link
Contributor Author

t0hai commented Sep 15, 2021

@midavadim docs & changelog have been updated

@marcosmarxm
Copy link
Member

Hi @n0rritt can you resolve the conflict in hubspot.md file? @midavadim can you review again?

@t0hai t0hai force-pushed the n0rritt/add_marketing_emails_stream branch 2 times, most recently from 0b2e80e to 10535e1 Compare September 24, 2021 11:22
@t0hai
Copy link
Contributor Author

t0hai commented Sep 24, 2021

@marcosmarxm I've rebased to current master and fixed all conflics
@midavadim it's ready for review

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2021

CLA assistant check
All committers have signed the CLA.

@marcosmarxm
Copy link
Member

Sorry the delay @n0rritt, can you sign the CLA?

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

@t0hai t0hai force-pushed the n0rritt/add_marketing_emails_stream branch from 10535e1 to 80ea5c6 Compare October 6, 2021 10:03
@t0hai
Copy link
Contributor Author

t0hai commented Oct 6, 2021

@marcosmarxm I've rebased to master again, can you check whether we can finally merge the PR please?

@t0hai t0hai force-pushed the n0rritt/add_marketing_emails_stream branch 2 times, most recently from d5cc9f0 to 907dc9a Compare October 7, 2021 08:17
@t0hai t0hai force-pushed the n0rritt/add_marketing_emails_stream branch from 907dc9a to 388d1e2 Compare October 13, 2021 09:40
@t0hai
Copy link
Contributor Author

t0hai commented Oct 13, 2021

@marcosmarxm could you check again whether we can finally merge this PR please?

@t0hai t0hai force-pushed the n0rritt/add_marketing_emails_stream branch from 388d1e2 to 7207229 Compare October 18, 2021 14:56
@marcosmarxm marcosmarxm merged commit fae4d00 into airbytehq:master Oct 20, 2021
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
…rbytehq#5840)

* Add new marketing emails (with statistics) stream

* Update docs and changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants