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

🚨🚨 Source Sendgrid: migrate to low code #35776

Merged
merged 48 commits into from
Apr 15, 2024
Merged

🚨🚨 Source Sendgrid: migrate to low code #35776

merged 48 commits into from
Apr 15, 2024

Conversation

bleonard
Copy link
Contributor

@bleonard bleonard commented Mar 3, 2024 •
edited
Loading

Migrates sendgrid to mostly low code.

  • Breaking changes

    • Spec changes (migrated)
      • apikey -> api_key
      • start_time -> start_date
      • start_date now required
    • unsubscribe_groups stream has been removed. It was the same as suppression_groups.
    • single_sends stream has been renamed singlesend_stats. This is closer to the data and API.
    • segments stream has been upgraded to use the new 2.0 API. The older one has been deprecated. The payload is a bit different.

  • Other changes

  • singlesends stream has been added to match the API.

@bleonard bleonard requested a review from a team March 3, 2024 05:06
@vercel Vercel
Copy link

vercel bot commented Mar 3, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2024 6:53am

@octavia-squidington-iv octavia-squidington-iv requested a review from a team March 3, 2024 05:07
natikgadzhi
natikgadzhi reviewed Mar 3, 2024
View reviewed changes
Copy link
Contributor

@natikgadzhi natikgadzhi left a comment •
edited by askarpets
Loading

Choose a reason for hiding this comment

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

I have not ran the connector myself yet. The code looks ok.

Notes:

  • In streams.py, parse_response might no longer be necessary. The only stream where it would be applied is Contacts, and that one downloads a CSV anyway. Let's remove it?
  • In streams.py, should_retry has the same fate. It would only apply to ContactsStream, not sure how useful it is for just this stream.
  • The dockerImageTag in metadata.yaml is 0.43 — I assume docker image tag should generally be the same as the version defined in manifest.yaml? Should we go with 1.0 for both? 🚂

Ideas:

  • Damn it would be good if you could click a button and open a cloud workspace with the yaml of this connector already loaded into it. Wouldn't quite work for Sendgrid because it's hybrid, but you get the idea — quickly jump to an environment where I can run a preview read and see if things work.

@natikgadzhi
Copy link
Contributor

Connector version is in three spots at least:

  • pyproject.toml: tool.poetry.version. Currently set to 0.4.3.
  • metadata.yaml: dockerImageTag.
  • source_sendgrid/manifest.yaml: version.

They should all be the same.

@alafanechere, what should we do about it? They should all be bumped consistently, and I would not be surprised if other connectors also have this borked. Can we apply the same principle we have in other spots with bumpversion.cfg?

When we get all low-code connectors to be just the manifest.yaml, we need to:

  • figure out a way to merge metadata.yaml and manifest.yaml so they're a single file
  • drop pyproject.toml when custom components are just python executed in CDK's python context without their own package environment.

At that point, we will only have one version declaration that would drive the other ones.

@natikgadzhi natikgadzhi mentioned this pull request Mar 3, 2024
Merged
lazebnyi
lazebnyi requested changes Mar 4, 2024
View reviewed changes
Copy link
Collaborator

@lazebnyi lazebnyi left a comment

Choose a reason for hiding this comment

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

I left some comments for pagination, dead code in unit tests and params in spec. Also need update PR name according to conventions, bump version of the connector and add note to changelog.

@octavia-squidington-iv octavia-squidington-iv requested a review from a team March 4, 2024 20:44
@askarpets askarpets requested a review from clnoll March 31, 2024 16:17
@octavia-squidington-iv octavia-squidington-iv requested a review from a team April 1, 2024 14:33
@octavia-squidington-iv octavia-squidington-iv requested a review from a team April 1, 2024 14:38
lazebnyi
lazebnyi approved these changes Apr 1, 2024
View reviewed changes
Copy link
Collaborator

@lazebnyi lazebnyi left a comment

Choose a reason for hiding this comment

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

lgtm

natikgadzhi
natikgadzhi approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

Nothing stands out as wrong to me — lgtm.

airbyte-integrations/bases/connector-acceptance-test/connector_acceptance_test/conftest.py
@@ -299,6 +299,8 @@ async def discovered_catalog_fixture(

output = await docker_runner.call_discover(config=connector_config)
catalogs = [message.catalog for message in output if message.type == Type.CATALOG]
if len(catalogs) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just merged from master, or is this new?

airbyte-integrations/connectors/source-sendgrid/source_sendgrid/manifest.yaml
- id
schema_loader:
type: InlineSchemaLoader
schema:
Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly, the formats are probably auto-generated by Builder. @lmossman, @girarda, do we already have something to track automatic format recognition?

clnoll
clnoll approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@clnoll clnoll left a comment

Choose a reason for hiding this comment

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

LGTM pending regression test results

@lazebnyi lazebnyi merged commit 3ed543b into master Apr 15, 2024
30 checks passed
@lazebnyi lazebnyi deleted the sendgrid_yaml branch April 15, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambirdsall ambirdsall ambirdsall left review comments

@askarpets askarpets askarpets left review comments

@lmossman lmossman lmossman left review comments

@tolik0 tolik0 tolik0 left review comments

@katmarkham katmarkham katmarkham approved these changes

@natikgadzhi natikgadzhi natikgadzhi approved these changes

@clnoll clnoll clnoll approved these changes

@lazebnyi lazebnyi lazebnyi approved these changes

@oustynova oustynova Awaiting requested review from oustynova

Assignees

@clnoll clnoll

Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/sendgrid low-code-migration This connector has been migrated to the low-code CDK team/connectors-python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.