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

feat: prepare_column() refactor and new activate_version config option. #240

Merged
merged 7 commits into from
Dec 28, 2023

Conversation

sebastianswms
Copy link
Contributor

Refactor of PostgresConnector's prepare_column() function to fall in line with SQLConnector's version of it.

Added activate_version config option, and related assertion to ensure that activate_version is not used unless add_record_metadata is enabled.

README.md Show resolved Hide resolved
README.md Outdated
| ssl_enable | False | 0 | Whether or not to use ssl to verify the server's identity. Use ssl_certificate_authority and ssl_mode for further customization. To use a client certificate to authenticate yourself to the server, use ssl_client_certificate_enable instead. Note if sqlalchemy_url is set this will be ignored. |
| ssl_client_certificate_enable| False | 0 | Whether or not to provide client-side certificates as a method of authentication to the server. Use ssl_client_certificate and ssl_client_private_key for further customization. To use SSL to verify the server's identity, use ssl_enable instead. Note if sqlalchemy_url is set this will be ignored. |
| ssl_mode | False | verify-full | SSL Protection method, see [postgres documentation](https://www.postgresql.org/docs/current/libpq-ssl.html#LIBPQ-SSL-PROTECTION) for more information. Must be one of disable, allow, prefer, require, verify-ca, or verify-full. Note if sqlalchemy_url is set this will be ignored. |
| ssl_certificate_authority | False | ~/.postgresql/root.crl | The certificate authority that should be used to verify the server's identity. Can be provided either as the certificate itself (in .env) or as a filepath to the certificate. Note if sqlalchemy_url is set this will be ignored. |
| ssl_client_certificate | False | ~/.postgresql/postgresql.crt | The certificate that should be used to verify your identity to the server. Can be provided either as the certificate itself (in .env) or as a filepath to the certificate. Note if sqlalchemy_url is set this will be ignored. |
| ssl_client_private_key | False | ~/.postgresql/postgresql.key | The private key for the certificate you provided. Can be provided either as the certificate itself (in .env) or as a filepath to the certificate. Note if sqlalchemy_url is set this will be ignored. |
| ssl_storage_directory | False | .secrets | The folder in which to store SSL certificates provided as raw values. When a certificate/key is provided as a raw value instead of as a filepath, it must be written to a file before it can be used. This configuration option determines where that file is created. |
| ssh_tunnel | False | None | SSH Tunnel Configuration, this is a json object |
| load_method | False | append-only | The method to use when loading data into the destination. `append-only` will always write all input records whether that records already exists or not. `upsert` will update existing records and insert new records. `overwrite` will delete all existing records and insert all input records. |
Copy link
Member

Choose a reason for hiding this comment

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

Is this new from the SDK? Pretty big change to folks defaults today this is just controlled by primary keys being set or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the SDK and included in --about. I agree it seems like a notable change.

self.full_table_name,
self.version_column_name, # type: ignore[arg-type]
sql_type=integer_type,
raise RuntimeError(
Copy link
Member

Choose a reason for hiding this comment

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

We do create this column somewhere right? I remember there being a bug here but I don't remember what it was exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I dove in here. Theoretically this error should never be hit because we always create the column, but I like it as a sanity check. If anything changes down the line, the error that would appear otherwise is not as intuitive.

Copy link
Member

Choose a reason for hiding this comment

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

Please add Theoretically this error should never be hit because we always create the column, but I like it as a sanity check. If anything changes down the line, the error that would appear otherwise is not as intuitive. as a comment then to the code here to explain this

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
target_postgres/target.py Outdated Show resolved Hide resolved
target_postgres/tests/test_target_postgres.py Show resolved Hide resolved
self.full_table_name,
self.version_column_name, # type: ignore[arg-type]
sql_type=integer_type,
raise RuntimeError(
Copy link
Member

Choose a reason for hiding this comment

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

Please add Theoretically this error should never be hit because we always create the column, but I like it as a sanity check. If anything changes down the line, the error that would appear otherwise is not as intuitive. as a comment then to the code here to explain this

target_postgres/tests/test_target_postgres.py Show resolved Hide resolved
@visch visch merged commit 25b8299 into MeltanoLabs:main Dec 28, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants