🐛 Add method field on spec.json connectors (snowflake and postgres)

[marcosmarxm]

What

Closes #3935 and I took the opportunity to closes #3950 too

How

Add method field when using oneOf property.

Recommended reading order

1. snowflake/spec.json
2. postres/spec.json

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

[marcosmarxm]

@jrhizor added you to evaluate if this change won't break back compatibility or if you have any suggestion about that.

[marcosmarxm]

Visiting the destination page after creating it.

Snowflake

Postgres

[marcosmarxm]

/test connector=source-postgres

🕑 source-postgres https://github.com/airbytehq/airbyte/actions/runs/919159503
❌ source-postgres https://github.com/airbytehq/airbyte/actions/runs/919159503

[marcosmarxm]

/test connector=destination-snowflake

🕑 destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/919159447
✅ destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/919159447

[sherifnada]

@marcosmarxm concerned about backwards incompatibility. Will this break configs for existing users?

[marcosmarxm]

@marcosmarxm concerned about backwards incompatibility. Will this break configs for existing users?

yes, we can remove method from being required but don't sure if this will ensure backwards incompatibility because we're using default values.

[marcosmarxm]

/test connector=source-postgres

🕑 source-postgres https://github.com/airbytehq/airbyte/actions/runs/919300624
✅ source-postgres https://github.com/airbytehq/airbyte/actions/runs/919300624

[cgardens]

@sherifnada can you walk me through what will be backwards incompatible? That's actually a bit of a trick question, because my hunch is that given some of the nuance of when we validate we won't have backwards incompatibility issues.

Here's how I think it works (for better or for worse). If we find out what the truth is here, we can then document it (and iterate if we don't like the existing behavior. Below do not assume validation happens unless I explicitly say it.

User creates / updates a connector

• User enters configuration for connector
• FE validates it
• Sent to the API.
• API validates it.
• API saves it.

Sync job runs

• config is fetched from db
• it is passed to the worker and executed (no validation happens)

So my hunch is that as long as the connector can run without the new field (which I think is true in this implementation), then already configured connectors will be fine.

[cgardens]

@marcosmarxm this makes sense to me! Thanks.

[marcosmarxm]

@marcosmarxm concerned about backwards incompatibility. Will this break configs for existing users?

@sherifnada and @cgardens :

1. I configured one Postgres using current version 0.3.2 using CDC with Snowflake as destination.
2. I accessed the Settings page and Standard was selected.
3. Ran sync (CDC process was called ok)
4. Update the source to dev tag, I think the same process users going to do after this release right?
5. Source didn't break but when I check the setting page the Replication Method continued as Standard.
6. I needed to change manually to CDC and update the name to update the source,
7. after that when accessing the settings page the Replication Method selected is CDC selected.

So, I think we wont have backwards incompatibility, but should have a way to warn users about this.

[sherifnada]

ty for the clarification marcos, shipit

[marcosmarxm]

/publish connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/919701788
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/919701788

[marcosmarxm]

/publish connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/919710477
❌ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/919710477

[michel-tricot]

it doesn't follow the convention we've been using so far to work around oneof

We need to be careful about this one as it is in spec.json. Meaning it becomes part of the connector iface.

[marcosmarxm]

it doesn't follow the convention we've been using so far to work around oneof

We need to be careful about this one as it is in spec.json. Meaning it becomes part of the connector iface.

@michel-tricot could you check @cgardens #3935 (comment). I think what I added is the workaround to deal with oneOf.
edited: Is the same for convention applied for destination-s3, file source connector destination-mssql too.

[michel-tricot]

Talked with @cgardens.

Good with the hack for now but we need to fix it ASAP before we lose control of it.

[marcosmarxm]

/publish connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/919756068
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/919756068

[marcosmarxm]

/publish connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/919790070
✅ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/919790070