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

🎉 New Source: Mailersend [low-code CDK] #18669

Merged
merged 9 commits into from
Nov 13, 2022
Merged

🎉 New Source: Mailersend [low-code CDK] #18669

merged 9 commits into from
Nov 13, 2022

Conversation

cirdes
Copy link
Contributor

@cirdes cirdes commented Oct 30, 2022

What

Developing new connector for the source: Mailersend

How

Developed using low-code cdk

Streams

  • activity

Recommended reading order

  1. spec.yaml
  2. mailersend.yaml

🚨 User Impact 🚨

No breaking changes, Just addition of new source

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • 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/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

Tests

Integration & Acceptance Screen Shot 2022-10-30 at 4 32 52 PM

Screen Shot 2022-10-30 at 4 38 23 PM

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.

Hello @cirdes, Marcos from Airbyte here 👋 . We received more than 25 new contributions along the weekend. One is yours 🎉 thank so much for! Our team is limited and maybe the review process can take longer than expected. As described in the Airbyte's Hacktoberfest your contribution was submitted before November 2nd and it is eligible to win the prize. The review process will validate other requirements. I ask to you patience until someone from the team review it.

Because I reviewed some contributions for Hacktoberfest so far I saw some common patterns you can check in advance:

  • Make sure you have added connector documentation to /docs/integrations/
  • Remove the file catalog from /integration_tests
  • Edit the sample_config.json inside /integration_tests
  • For the configured_catalog you can use only json_schema: {}
  • Add title to all properties in the spec.yaml
  • Make sure the documentationUrl in the spec.yaml redirect to Airbyte's future connector page, eg: connector Airtable the documentationUrl: https://docs.airbyte.com/integrations/sources/airtable
  • Review now new line at EOF (end-of-file) for all files.

If possible send to me a DM in Slack with the tests credentials, this process will make easier to us run integration tests and publish your connector. If you only has production keys, make sure to create a bootstrap.md explaining how to get the keys.

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.

Some comments.

Comment on lines 20 to 29
date_from:
type: string
description: Timestamp is assumed to be UTC. Must be lower than date_to.
examples:
- "1443651141"
date_to:
type: string
description: Timestamp is assumed to be UTC. Must be higher than date_from.
examples:
- "1443651141"
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename this to start_date and end_date?

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, date_from and date_to is how mailersend named it on docs

Screenshot 2022-10-31 at 7 43 57 PM

Should I renamed it?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can rename because the user don't need to know the name the API uses, probably is better to use a more common terminology inside Airbyte.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it!

docs/integrations/README.md Outdated Show resolved Hide resolved
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.

@cirdes two comments, one about the parameters and other about the incremental implementation.

Comment on lines +38 to +59
activity_stream:
$ref: "*ref(definitions.base_stream)"
$options:
name: "activity"
primary_key: "id"
path: "/activity/{{ config['domain_id'] }}"
Copy link
Member

Choose a reason for hiding this comment

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

I saw the API supports the query parameter from/to using dates, this allow using incremental syncs. @cirdes what about implementing this feature for this connector? I can help you if you need some help.

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, the API does supports query parameter using dates. But it expect the params to be a integer like 1443651141 (utc in seconds). DatetimeStreamSlicer is using date as strings "2021-02-01T00:00:00.000000+0000".

Is it possible to use DatetimeStreamSlicer in this case?

Copy link
Member

Choose a reason for hiding this comment

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

@girarda can you have me if it is possible to convert to timestamp?

Copy link
Contributor

Choose a reason for hiding this comment

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

you can set the date format to %s to convert it to a timestamp. the sendgrid connector is an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks guy, I will have a look on it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if a got the point here.

The challenge is not about the stream_slicer itself.
In the spec.yml I'm asking the user to enter start_time(or date_from) as a number because I need to send start_time as number to Mailersend's API.

mailersend.yml

request_options_provider:
  request_parameters:
    limit: "100"
    start_time: "{{ config['date_from'] }}"

But if a change start_time to be a string link "2022-11-05" I will need to cast the string into Date and then get the unix timestamp but its seems to be not possible in the yml.
In sendgrid exemple it is doing that using python and pendulum lib -> https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-sendgrid/source_sendgrid/streams.py#L98

It's seems to be a complex scenario or I'm missing a simple solution here?

cc: @marcosmarxm

Copy link
Contributor

Choose a reason for hiding this comment

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

@cirdes, you can configure the datetime format of the stream slicer's fields. The %s directive converts the datetime to a timestamp.

Using Sendgrid as an example:

  stream_slicer:
    type: "DatetimeStreamSlicer"
    start_datetime:
      datetime: "{{ config['start_time'] }}"
      datetime_format: "%s"

https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-sendgrid/source_sendgrid/sendgrid.yaml#L47-L63

Copy link
Member

Choose a reason for hiding this comment

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

@cirdes let's schedule a time tomorrow to implement the stream slicer.

@marcosmarxm
Copy link
Member

Hello! I'm going to be out of the office this Friday and won't be able to review your contribution again today, I return next Monday. So far, most contributions look solid and are almost done to be approved. As said in Chris' comment all contributions made before 2-November are eligible to receive the prize and have 2 weeks to merge the contributions. But I ensure next week we're going to have your contribution merged. If you have questions about the implementation you can send them in #hacktoberfest-2022 in Airbyte's Slack.

Sorry the inconvenience and see you again next week, thank you so much for your contribution!

request_options_provider:
request_parameters:
limit: "100"
stream_slicer:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@girarda , I'm trying to add stream slicer support but I'm getting this error:

{"message": "Something went wrong in the connector. See the logs for more details.", "internal_message": "invalid literal for int() with base 10: '2022-11-06'", "stack_trace": "Traceback (most recent call last):\n  File \"/Users/cirdes/Workspace/airbyte/airbyte-integrations/connectors/source-mailersend/main.py\", line 13

The api should be called like this:
https://api.mailersend.com/v1/activity/{domain_id}/?date_from=1667746844
date_from is the request_parameters and it's a Unix Timestamp (integer)

in spec.yml I have start_date

start_date:
  type: string
  description: Timestamp is assumed to be UTC. Must be lower than date_to.
  examples:
    - "2022-11-06"

And I'm using "2022-11-06" as start_date.

@marcosmarxm
Copy link
Member

marcosmarxm commented Nov 13, 2022

/test connector=connectors/source-mailersend

🕑 connectors/source-mailersend https://github.com/airbytehq/airbyte/actions/runs/3457020316
❌ connectors/source-mailersend https://github.com/airbytehq/airbyte/actions/runs/3457020316
🐛 https://gradle.com/s/44sozabnqvwas

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs0] - Failed: Timeout >300.0s
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: This connector does not implement incremental sync
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:88: The previous connector image could not be retrieved.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:358: The previous connector image could not be retrieved.
============= 1 failed, 25 passed, 3 skipped in 1342.84s (0:22:22) =============

@marcosmarxm
Copy link
Member

marcosmarxm commented Nov 13, 2022

/test connector=connectors/source-mailersend

🕑 connectors/source-mailersend https://github.com/airbytehq/airbyte/actions/runs/3457145521
✅ connectors/source-mailersend https://github.com/airbytehq/airbyte/actions/runs/3457145521
Python tests coverage:

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       139      5    96%   87, 93, 235, 239-240
	 source_acceptance_test/conftest.py                     196     92    53%   35, 41-43, 48, 54, 60, 66, 72-74, 93, 98-100, 106-108, 114-115, 120-121, 126, 132, 141-150, 156-161, 176, 200, 231, 237, 243-248, 256-261, 269-282, 287-293, 300-311, 318-334
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              394    111    72%   53, 58, 87-95, 100-107, 111-112, 116-117, 293, 331-348, 357-365, 369-374, 380, 413-418, 456-463, 506-508, 511, 576-584, 596-599, 604, 660-661, 667, 670, 706-716, 729-754
	 source_acceptance_test/tests/test_incremental.py       158     14    91%   52-59, 64-77, 240
	 source_acceptance_test/utils/asserts.py                 37      2    95%   57-58
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       112     50    55%   23-26, 32, 36, 39-68, 71-73, 76-78, 81-83, 86-88, 91-93, 96-114, 148-150
	 source_acceptance_test/utils/json_schema_helper.py     105     13    88%   30-31, 38, 41, 65-68, 96, 120, 190-192
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1557    349    78%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: This connector does not implement incremental sync
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:88: The previous connector image could not be retrieved.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:358: The previous connector image could not be retrieved.
================== 26 passed, 3 skipped in 189.27s (0:03:09) ===================

@marcosmarxm
Copy link
Member

marcosmarxm commented Nov 13, 2022

/publish connector=connectors/source-mailersend

🕑 Publishing the following connectors:
connectors/source-mailersend
https://github.com/airbytehq/airbyte/actions/runs/3457218900


Connector Did it publish? Were definitions generated?
connectors/source-mailersend

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

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.

thanks @cirdes

akashkulk pushed a commit that referenced this pull request Dec 2, 2022
* New source Mailersend

* first review

* trying to add stream_slicer

* change spec.yml

* add streamslicer

* Update spec.yaml

* add mailersend to source def

* auto-bump connector version

Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants