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

SSH for Postgres Destination #5743

Merged
merged 12 commits into from
Sep 8, 2021
Merged

SSH for Postgres Destination #5743

merged 12 commits into from
Sep 8, 2021

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Aug 30, 2021

What

  • Follows on work from SSH Postgres Source SSH for Postgres Source #5742
  • @Phlair as far as I can tell, all tests are passing except those that require normalization. Can you rebase onto this branch so that we can get to green and release this.

How

  • Slightly different approach than I took on the Postgres Source. I use a decorate to wrap the destination methods in SSH tunnels. I think I like this pattern better. Curious to hear opinions.

Recommended reading order

  1. SshDestination.java
  2. SshWrappedJdbcDestination.java

Pre-merge Checklist

  • verify normalization works
  • publish version
  • update public docs and change log
  • update developer logs

@cgardens
Copy link
Contributor Author

cgardens commented Aug 30, 2021

/test connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/1183945013
❌ connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/1183945013

@github-actions github-actions bot added the area/connectors Connector related issues label Aug 30, 2021
@jrhizor jrhizor temporarily deployed to more-secrets August 30, 2021 22:21 Inactive
Copy link
Contributor

@airbyte-jenny airbyte-jenny left a comment

Choose a reason for hiding this comment

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

The decorator wrapping approach seems clever and pretty clean. Code makes sense overall.


private final Destination delegate;

public SshWrappedJdbcDestination(final Destination delegate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the name of the class, if it is only for JDBC destination, shoudn't the argument be of type JdbcDestination or a less permissive type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I actually think the name of this class should be more permissive. Just call it SshWrappedDestination. Ultimately there's nothing here going on that can't work for anything that we might want to access via ssh tunnel.

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.

I have the same question about transparently replacing hostname, but otherwise looks mergable

Base automatically changed from cgardens/pg_ssh to master September 2, 2021 18:32
@cgardens cgardens force-pushed the cgardens/pg_ssh_destination branch from 817f1fa to d43b500 Compare September 3, 2021 14:38
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Sep 3, 2021
@cgardens
Copy link
Contributor Author

cgardens commented Sep 3, 2021

/test connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/1198429643
❌ connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/1198429643

@jrhizor jrhizor temporarily deployed to more-secrets September 3, 2021 14:54 Inactive
@cgardens cgardens force-pushed the cgardens/pg_ssh_destination branch from e63a5a3 to 7df041f Compare September 7, 2021 21:12
@github-actions github-actions bot added area/worker Related to worker normalization labels Sep 7, 2021
@cgardens
Copy link
Contributor Author

cgardens commented Sep 7, 2021

/test connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/1211125237
❌ connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/1211125237

@jrhizor jrhizor temporarily deployed to more-secrets September 7, 2021 22:01 Inactive
@cgardens
Copy link
Contributor Author

cgardens commented Sep 7, 2021

/publish connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1211182403
✅ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1211182403

@jrhizor jrhizor temporarily deployed to more-secrets September 7, 2021 22:23 Inactive
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.

mostly reviewed the normalization changes, lgtm

docs/integrations/sources/postgres.md Outdated Show resolved Hide resolved
@jrhizor jrhizor temporarily deployed to more-secrets September 7, 2021 23:10 Inactive
@cgardens
Copy link
Contributor Author

cgardens commented Sep 7, 2021

/publish connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/1211306660
✅ connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/1211306660

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 area/worker Related to worker normalization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants