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

destination-dev-null: merge cloud and OSS #45651

Open
wants to merge 1 commit into
base: stephane/09-18-destination-e2e_upgrade_cdk
Choose a base branch
from

Conversation

stephane-airbyte
Copy link
Contributor

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

  • YES πŸ’š
  • NO ❌

Copy link

vercel bot commented Sep 18, 2024

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 Sep 19, 2024 10:20pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Sep 18, 2024
Copy link
Contributor Author

stephane-airbyte commented Sep 18, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @stephane-airbyte and the rest of your teammates on Graphite Graphite

@@ -2,7 +2,7 @@ data:
connectorSubtype: unknown
connectorType: destination
definitionId: 2eb65e87-983a-4fd7-b3e3-9d9dc6eb8537
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, if we drop the cloud version but don't change the definitionId or name of the oss version, then we effectively create a new cloud connector with a new name? That is what we want? All existing connections using dev-null will just be parked at the old version until they switch over?

@stephane-airbyte stephane-airbyte force-pushed the stephane/09-18-destination-e2e_merge_cloud_and_oss branch from afa13f1 to 8843925 Compare September 18, 2024 21:32
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-18-destination-e2e_upgrade_cdk branch from 0af4c79 to 5e96375 Compare September 18, 2024 21:47
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-18-destination-e2e_merge_cloud_and_oss branch from 8843925 to 9d66c12 Compare September 18, 2024 21:48
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-18-destination-e2e_upgrade_cdk branch from 5e96375 to 2946cca Compare September 18, 2024 21:58
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-18-destination-e2e_merge_cloud_and_oss branch from 9d66c12 to a657ecb Compare September 18, 2024 21:58
@stephane-airbyte stephane-airbyte changed the title destination-e2e: merge cloud and OSS destination-dev-null: merge cloud and OSS Sep 18, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-18-destination-e2e_merge_cloud_and_oss branch from a657ecb to 00b415b Compare September 18, 2024 22:21
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-18-destination-e2e_upgrade_cdk branch from 2946cca to 8a624a3 Compare September 18, 2024 22:46
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-18-destination-e2e_merge_cloud_and_oss branch from 00b415b to 77d7133 Compare September 18, 2024 22:46
@stephane-airbyte stephane-airbyte marked this pull request as ready for review September 19, 2024 00:00
@@ -42,6 +48,33 @@ constructor(
]
}

override fun spec(): ConnectorSpecification {
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 backwards? You remove everything but silent from the !isCloudDeployment version

/** 1. Update the title. 2. Only keep the "silent" mode. */
val spec = super.spec()

(spec.connectionSpecification as ObjectNode).put("title", DEV_NULL_DESTINATION_TITLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the cloud title also?

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 guess I fucked that one up? I reverted them. I'll fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, why should we use different names, now that they're the same connector?

Copy link
Contributor

Choose a reason for hiding this comment

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

🀷 Minimal disruption? Keeping parity with what exists?

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 CDK Connector Development Kit connectors/destination/dev-null connectors/destination/e2e-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants