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.

Sign up for GitHub

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

Jump to bottom

🎉 Rewrite sendgrid source using HTTP CDK #3445

Merged
merged 12 commits into from
May 20, 2021
Merged

🎉 Rewrite sendgrid source using HTTP CDK #3445

merged 12 commits into from
May 20, 2021

Conversation

htrueman
Copy link
Contributor

What

Update sendgrid source to use current system-wide coding style. Update source and stream classes.

How

  • Removed deprecated airbyte-protocol and base-python dependencies.
  • Updated imports to use airbyte_cdk library.
  • Updated related files (docker, readme etc.)
  • Created airbyte-cdk source and stream classes
  • Removed old source and client classes

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. Imports in python files
  2. Dockerfile, requirements.txt, setup.py, main.py and README.md
  3. source_sendgrid/source.py and source_sendgrid/api.py for updated source and stream classes

This was linked to issues May 17, 2021
Source Iterable: update to use latest version of CDK #3044
Closed
Rewrite sendgrid source using HTTP CDK #3255
Closed
@htrueman htrueman removed a link to an issue May 17, 2021
Source Iterable: update to use latest version of CDK #3044
Closed
@htrueman
Fix sendgrid source streams, check_connection.
1b361f9 
Fix test_source_wrong_credentials.
Add source-acceptance-test config.
@htrueman htrueman requested a review from keu May 18, 2021 14:03
sherifnada
sherifnada reviewed May 18, 2021
View reviewed changes
airbyte-integrations/connectors/source-sendgrid/setup.py Outdated
@@ -31,6 +31,6 @@
author="Airbyte",
author_email="contact@airbyte.io",
packages=find_packages(),
install_requires=["airbyte-protocol", "base-python", "backoff", "requests", "pytest==6.1.2"],
install_requires=["airbyte-cdk==0.1.3", "backoff", "requests", "pytest==6.1.2"],
Copy link
Contributor

Choose a reason for hiding this comment

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

probably better to do something like ~=0.1 to pick up backwards compatible changes

airbyte-integrations/connectors/source-sendgrid/source_sendgrid/api.py Outdated Show resolved Hide resolved
@htrueman
Update setup requirements airbyte-cdk version spec.
6b44ea7 
Fix Templates stream request_params.
keu
keu suggested changes May 19, 2021
View reviewed changes
Copy link
Contributor

@keu keu left a comment

Choose a reason for hiding this comment

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

very small changes

airbyte-integrations/connectors/source-sendgrid/source_sendgrid/api.py Outdated
@@ -0,0 +1,201 @@
#
Copy link
Contributor

@keu keu May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

let's rename it to streams.py

airbyte-integrations/connectors/source-sendgrid/Dockerfile Outdated Show resolved Hide resolved
sherifnada
sherifnada approved these changes May 19, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

LGTM w comments from eugene

@htrueman
Copy link
Contributor Author

htrueman commented May 20, 2021 •
edited by github-actions bot
Loading

/test connector=source-sendgrid

🕑 source-sendgrid https://github.com/airbytehq/airbyte/actions/runs/860146464
❌ source-sendgrid https://github.com/airbytehq/airbyte/actions/runs/860146464

@htrueman htrueman changed the title Rewrite sendgrid source using HTTP CDK 🎉 Rewrite sendgrid source using HTTP CDK May 20, 2021
@htrueman
Copy link
Contributor Author

htrueman commented May 20, 2021 •
edited by github-actions bot
Loading

/test connector=source-sendgrid

🕑 source-sendgrid https://github.com/airbytehq/airbyte/actions/runs/860194571
❌ source-sendgrid https://github.com/airbytehq/airbyte/actions/runs/860194571

@htrueman
Copy link
Contributor Author

@keu @sherifnada @yevhenii-ldv could not fill spam_reports stream, so tests won't pass.
Should I publish as it?

@sherifnada
Copy link
Contributor

@htrueman you can exclude it from configured catalog used for testing for now I believe cc @keu

Sgtm

@htrueman
Add supported_sync_modes to configured catalogs.
7152766 
Add no_spam_reports_configured_catalog.json with excluded spam_reports from acceptance test.
Update acceptance-test-config.yml to use no_spam_reports_configured_catalog.json in basic_read.
@htrueman
Copy link
Contributor Author

htrueman commented May 20, 2021 •
edited by github-actions bot
Loading

/test connector=source-sendgrid

🕑 source-sendgrid https://github.com/airbytehq/airbyte/actions/runs/860690605
✅ source-sendgrid https://github.com/airbytehq/airbyte/actions/runs/860690605

@htrueman
Copy link
Contributor Author

htrueman commented May 20, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-sendgrid

🕑 connectors/source-sendgrid https://github.com/airbytehq/airbyte/actions/runs/860751551
✅ connectors/source-sendgrid https://github.com/airbytehq/airbyte/actions/runs/860751551

@htrueman htrueman merged commit 5ea454a into master May 20, 2021
@htrueman htrueman deleted the htrueman/rewrite-sendgrid-source-using-cdk branch May 20, 2021 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@po3na4skld po3na4skld po3na4skld approved these changes

@sherifnada sherifnada sherifnada approved these changes

@keu keu keu approved these changes

@grebessi grebessi Awaiting requested review from grebessi

@Zirochkaa Zirochkaa Awaiting requested review from Zirochkaa

@yevhenii-ldv yevhenii-ldv Awaiting requested review from yevhenii-ldv

@vovavovavovavova vovavovavovavova Awaiting requested review from vovavovavovavova

@vitaliizazmic vitaliizazmic Awaiting requested review from vitaliizazmic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rewrite sendgrid source using HTTP CDK
4 participants
@htrueman @sherifnada @keu @po3na4skld