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

Simplify Github and Postgres forms #2 #24255

Merged
merged 17 commits into from
Mar 22, 2023

Conversation

lmossman
Copy link
Contributor

@lmossman lmossman commented Mar 20, 2023

What

Resolves #23696

As part of the simplify project definition of done, we want to migration a couple of connectors to use the new fields.

There was a previous PR up to address this, but it was accidentally merged before the tests were properly updated, so this PR is the replacement for that one.

How

Migrates the postgres and github source connectors to use the new simplify fields (group and groups), so that they match the designs linked in the above issue.

The Postgres connector was a bit trickier to migrate, because it dynamically creates its spec by merging in the ssh-tunnel-spec.json through the SshHelpers class.
So, in order to set a group on the tunnel_method field of the postgres spec (so that the field is placed in the right place on the UI), I had to update this class to also take in an optional group parameter and set it on the tunnel_method if present, which is set to "security" in the Postgres source connector.

As a result I also had to update the acceptance tests accordingly. To make this change slightly more maintainable, I created an AbstractPostgresSourceAcceptanceTest class which overrides the getSpec method with the right group setting, and made the other acceptance tests extend that one (except the strict encrypt one since it does things slightly differently and it already extends another class).

Recommended reading order

  1. */spec.json - Update the github and postgres connectors to add group and groups to the fields to make them look like the designs in the linked tie
  2. SshHelpers.java - add the overloaded methods that take in an optional group argument, and set that on the tunnel method if present.
  3. SshWrappedSource.java - add an overloaded constructor that takes in a group and passes it to SshHelpers if provided.
  4. PostgresSource.java - use the new constructor to pass the ssh group in
  5. */Dockerfile - bump the versions in the dockerfiles
  6. AbstractPostgresSourceAcceptanceTest.java - define a new abstract class that sets the image name and spec as it is set in all of the other acceptance tests, and update them to extend this
  7. expected_spec.json - update the expected spec accordingly

🚨 User Impact 🚨

The postgres and github source connectors will have a simplified look for users, only if they have the connector.form.simplifyConfiguration feature flag enabled.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2023

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

  • Run integration tests
  • Bump connector or module version
  • Add changelog
  • Publish the new version

✅ Sources (31)

Connector Version Changelog Publish
source-alloydb 2.0.6
source-alloydb-strict-encrypt 2.0.6 🔵
(ignored)
🔵
(ignored)
source-bigquery 0.2.3
source-clickhouse 0.1.16
source-clickhouse-strict-encrypt 0.1.16 🔵
(ignored)
🔵
(ignored)
source-cockroachdb 0.1.21
source-cockroachdb-strict-encrypt 0.1.21 🔵
(ignored)
🔵
(ignored)
source-db2 0.1.18
source-db2-strict-encrypt 0.1.18 🔵
(ignored)
🔵
(ignored)
source-dynamodb 0.1.2
source-e2e-test 2.1.4
source-e2e-test-cloud 2.1.4 🔵
(ignored)
🔵
(ignored)
source-elasticsearch 0.1.1
source-jdbc 0.3.5 🔵
(ignored)
🔵
(ignored)
source-kafka 0.2.3
source-mongodb-strict-encrypt 0.1.19 🔵
(ignored)
🔵
(ignored)
source-mongodb-v2 0.1.19
source-mssql 1.0.5
source-mssql-strict-encrypt 1.0.5 🔵
(ignored)
🔵
(ignored)
source-mysql 2.0.7
source-mysql-strict-encrypt 2.0.7 🔵
(ignored)
🔵
(ignored)
source-oracle 0.3.23
source-oracle-strict-encrypt 0.3.23 🔵
(ignored)
🔵
(ignored)
source-postgres 2.0.8
source-postgres-strict-encrypt 2.0.8 🔵
(ignored)
🔵
(ignored)
source-redshift 0.3.16
source-relational-db 0.3.1 🔵
(ignored)
🔵
(ignored)
source-scaffold-java-jdbc 0.1.0 🔵
(ignored)
🔵
(ignored)
source-sftp 0.1.2
source-snowflake 0.1.31
source-tidb 0.2.3
  • See "Actionable Items" below for how to resolve warnings and errors.

❌ Destinations (48)

Connector Version Changelog Publish
destination-azure-blob-storage 0.2.0
destination-bigquery 1.2.17
destination-bigquery-denormalized 1.2.17
destination-cassandra 0.1.4
destination-clickhouse 0.2.2
destination-clickhouse-strict-encrypt 0.2.2 🔵
(ignored)
🔵
(ignored)
destination-csv 1.0.0
destination-databricks 1.0.0
destination-dev-null 0.2.7 🔵
(ignored)
🔵
(ignored)
destination-doris 0.1.0
destination-dynamodb 0.1.7
destination-e2e-test 0.2.4
destination-elasticsearch 0.1.6
destination-elasticsearch-strict-encrypt 0.1.6 🔵
(ignored)
🔵
(ignored)
destination-exasol 0.1.1
destination-gcs 0.2.16
destination-iceberg 0.1.0
destination-jdbc 0.3.14 🔵
(ignored)
🔵
(ignored)
destination-kafka 0.1.10
destination-keen 0.2.4
destination-kinesis 0.1.5
destination-local-json 0.2.11
destination-mariadb-columnstore 0.1.7
destination-mongodb 0.1.9
destination-mongodb-strict-encrypt 0.1.9 🔵
(ignored)
🔵
(ignored)
destination-mqtt 0.1.3
destination-mssql 0.1.22
destination-mssql-strict-encrypt 0.1.22 🔵
(ignored)
🔵
(ignored)
destination-mysql 0.1.20
destination-mysql-strict-encrypt 0.1.21
(mismatch: 0.1.20)
🔵
(ignored)
🔵
(ignored)
destination-oracle 0.1.19
destination-oracle-strict-encrypt 0.1.19 🔵
(ignored)
🔵
(ignored)
destination-postgres 0.3.26
destination-postgres-strict-encrypt 0.3.26 🔵
(ignored)
🔵
(ignored)
destination-pubsub 0.2.0
destination-pulsar 0.1.3
destination-r2 0.1.0
destination-redis 0.1.4
destination-redpanda 0.1.0
destination-redshift 0.4.3
destination-rockset 0.1.4
destination-s3 0.3.22
destination-s3-glue 0.1.3
destination-scylla 0.1.3
destination-snowflake 0.4.56
destination-teradata 0.1.1
destination-tidb 0.1.0
destination-yugabytedb 0.1.1
  • See "Actionable Items" below for how to resolve warnings and errors.

✅ Other Modules (0)

Actionable Items

(click to expand)

Category Status Actionable Item
Version
mismatch
The version of the connector is different from its normal variant. Please bump the version of the connector.

doc not found
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug.
Changelog
doc not found
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug.

changelog missing
There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog.
Publish
not in seed
The connector is not in the seed file (e.g. source_definitions.yaml), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug.

diff seed version
The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the /publish command to publish the latest version.

import io.airbyte.protocol.models.v0.ConnectorSpecification;
import java.util.Optional;

public abstract class AbstractPostgresSourceAcceptanceTest extends SourceAcceptanceTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created this abstract class so that I didn't need to repeat the "security" group string in every test

@@ -1,383 +0,0 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted this file because it was unused, and I wasted some time figuring out which expected_spec.json file I was supposed to update

@lmossman
Copy link
Contributor Author

lmossman commented Mar 20, 2023

/test connector=connectors/source-github

🕑 connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/4473674269
✅ connectors/source-github https://github.com/airbytehq/airbyte/actions/runs/4473674269
Python tests coverage:

Name                             Stmts   Miss  Cover
----------------------------------------------------
source_github/utils.py              14      0   100%
source_github/github_schema.py    8807      0   100%
source_github/__init__.py            2      0   100%
source_github/streams.py           854     91    89%
source_github/graphql.py           167     22    87%
source_github/source.py            118     34    71%
----------------------------------------------------
TOTAL                             9962    147    99%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:509: The previous and actual discovered catalogs are identical.
================== 45 passed, 2 skipped in 326.30s (0:05:26) ===================

@lmossman
Copy link
Contributor Author

lmossman commented Mar 20, 2023

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4473675887
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4473675887
🐛 https://gradle.com/s/xneomdt57rl2c

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestSpec::test_match_expected[inputs0] - AssertionError:...
FAILED test_core.py::TestSpec::test_match_expected[inputs1] - AssertionError:...
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:509: The previous and actual discovered catalogs are identical.
============== 2 failed, 62 passed, 3 skipped in 99.96s (0:01:39) ==============

@lmossman lmossman changed the title Simplify Github and Postgres forms (attempt 2) Simplify Github and Postgres forms #2 Mar 20, 2023
@lmossman
Copy link
Contributor Author

lmossman commented Mar 20, 2023

/test connector=connectors/source-postgres-strict-encrypt

🕑 connectors/source-postgres-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/4473679681
✅ connectors/source-postgres-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/4473679681
No Python unittests run

Build Passed

Test summary info:

All Passed

@lmossman
Copy link
Contributor Author

lmossman commented Mar 21, 2023

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4474067450
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4474067450
No Python unittests run

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:509: The previous and actual discovered catalogs are identical.
=================== 64 passed, 3 skipped in 98.97s (0:01:38) ===================

@lmossman lmossman marked this pull request as ready for review March 21, 2023 00:18
@lmossman lmossman requested a review from a team as a code owner March 21, 2023 00:18
@lmossman lmossman requested a review from a team as a code owner March 21, 2023 00:18
@lmossman
Copy link
Contributor Author

@flash1293 @rodireich I have put up this new PR to replace the old one that we reverted, and all of the acceptance tests are now passing, so this is ready for a review.

Once this gets approval, I will run the /publish commands for github, postgres, and postgres-strict-encrypt, then I'll merge this in

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Build github and postgres locally and tested - LGTM. The only thing we might need to address is to allow collapsing of oneOfs:
Screenshot 2023-03-21 at 11 55 46

But that's a separate discussion

@lmossman
Copy link
Contributor Author

lmossman commented Mar 22, 2023

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4492546043
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4492546043
No Python unittests run

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:509: The previous and actual discovered catalogs are identical.
================== 64 passed, 3 skipped in 104.60s (0:01:44) ===================

@lmossman
Copy link
Contributor Author

lmossman commented Mar 22, 2023

/test connector=connectors/source-postgres-strict-encrypt

🕑 connectors/source-postgres-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/4492551103
✅ connectors/source-postgres-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/4492551103
No Python unittests run

Build Passed

Test summary info:

All Passed

@lmossman
Copy link
Contributor Author

lmossman commented Mar 22, 2023

/publish connector=connectors/source-github

🕑 Publishing the following connectors:
connectors/source-github
https://github.com/airbytehq/airbyte/actions/runs/4492724689


Connector Did it publish? Were definitions generated?
connectors/source-github

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@lmossman
Copy link
Contributor Author

lmossman commented Mar 22, 2023

/publish connector=connectors/source-postgres

🕑 Publishing the following connectors:
connectors/source-postgres
https://github.com/airbytehq/airbyte/actions/runs/4492730243


Connector Did it publish? Were definitions generated?
connectors/source-postgres

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@lmossman
Copy link
Contributor Author

lmossman commented Mar 22, 2023

/publish connector=connectors/source-postgres-strict-encrypt

🕑 Publishing the following connectors:
connectors/source-postgres-strict-encrypt
https://github.com/airbytehq/airbyte/actions/runs/4492732028


Connector Did it publish? Were definitions generated?
connectors/source-postgres-strict-encrypt

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@lmossman
Copy link
Contributor Author

lmossman commented Mar 22, 2023

/publish connector=connectors/source-postgres-strict-encrypt

🕑 Publishing the following connectors:
connectors/source-postgres-strict-encrypt
https://github.com/airbytehq/airbyte/actions/runs/4492788301


Connector Did it publish? Were definitions generated?
connectors/source-postgres-strict-encrypt

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@lmossman lmossman merged commit b1fbd6f into master Mar 22, 2023
@lmossman lmossman deleted the lmossman/simplify-postgres-and-github-forms branch March 22, 2023 18:56
erohmensing pushed a commit that referenced this pull request Mar 22, 2023
* add grouping and collapsing fields to postgres source

* add auth group to github source connector

* revert postgres field order changes and adjust group of schemas field

* inject group into ssh tunnel spec for postgres only, through overloaded methods

* Automated Change

* bump Dockerfile versions and update changelogs

* bump strict encrypt version as well

* fix postgres acceptance test

* fix acceptance test again

* fix all postgres acceptance tests

* add newline

* undo other changes to postgres readme file

* add security group to tunnel_method in expected_spec.json

* bump version of strict encrypt

* manually bump versions in seed files

---------

Co-authored-by: lmossman <lmossman@users.noreply.github.com>
erohmensing pushed a commit that referenced this pull request Mar 22, 2023
* add grouping and collapsing fields to postgres source

* add auth group to github source connector

* revert postgres field order changes and adjust group of schemas field

* inject group into ssh tunnel spec for postgres only, through overloaded methods

* Automated Change

* bump Dockerfile versions and update changelogs

* bump strict encrypt version as well

* fix postgres acceptance test

* fix acceptance test again

* fix all postgres acceptance tests

* add newline

* undo other changes to postgres readme file

* add security group to tunnel_method in expected_spec.json

* bump version of strict encrypt

* manually bump versions in seed files

---------

Co-authored-by: lmossman <lmossman@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate 1-2 connectors to new simplify format
3 participants