-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Publish SSL-only version of Postgres Destination #6496
Publish SSL-only version of Postgres Destination #6496
Conversation
/publish connector=bases/base-normalization |
/publish connector=bases/base-normalization |
@@ -174,6 +174,10 @@ def transform_postgres(config: Dict[str, Any]): | |||
"threads": 32, | |||
} | |||
|
|||
# if unset, we assume true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully this is all that is needed in normalization. still running tests.
/publish connector=bases/base-normalization
|
# if unset, we assume true. | ||
if config.get("ssl", True): | ||
config["sslmode"] = "require" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From DBT docs:
https://docs.getdbt.com/reference/warehouse-profiles/postgres-profile#sslmode
When unset, dbt will connect to databases using the Postgres default, prefer, as the sslmode.
Shouldn't we explicitly set it to "require" if it is unset in the destination config then?
# if unset, we assume true. | |
if config.get("ssl", True): | |
config["sslmode"] = "require" | |
if "ssl" not in config or config.get("ssl", True): | |
config["sslmode"] = "require" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran /test commands and they work now!
We're just waiting for an answer from this question before publishing then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline. decided the original syntax is okay. if "config" is not set then get will default to true.
db = new PostgreSQLContainer<>(DockerImageName.parse("marcosmarxm/postgres-ssl:dev").asCompatibleSubstituteFor("postgres")) | ||
.withCommand("postgres -c ssl=on -c ssl_cert_file=/var/lib/postgresql/server.crt -c ssl_key_file=/var/lib/postgresql/server.key"); | ||
db.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Postgres database container is started for normalization here:
airbyte/airbyte-integrations/bases/base-normalization/integration_tests/dbt_integration_test.py
Line 38 in 3369237
print("Starting localhost postgres container for tests") |
I am guessing, it should be started with SSL using the same image there too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed this change into normalization integration test on your branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
/test connector=bases/base-normalization
|
/test connector=bases/base-normalization
|
/publish connector=bases/base-normalization
|
/publish connector=bases/base-normalization
|
I am merging this for now so it's not blocking others that need to bump version of normalization since the docker image has already been published I think we still need to publish the postgres strict connector too? @cgardens |
thanks. yeah. |
Relates to #6332
What
How
Recommended reading order
transform.py
PostgresDestinationStrictEncrypt.java
Pre-merge Checklist
(blocked on github actions outage)(blocked on github actions outage)