Skip to content
This repository was archived by the owner on May 2, 2023. It is now read-only.

Conversation

@jakedave
Copy link

@jakedave jakedave commented Mar 22, 2023

Relating to @dlawin ask to move code from this PR downstream as to increase DRY-ness

Using poetry run python3 -m data_diff --dbt with a password (incorrect due to our org's restriction on un/pw) from a profile.yml results in:

[13:26:08] ERROR - 250001 (08001): Failed to connect to DB: xxx.us-east-1.snowflakecomputing.com:443. Incorrect username or password was specified.

running the same command while specifying a private_key_path and private_key_passphrase results in

[13:18:29] ERROR - Bad decrypt. Incorrect password?

or, with correct key auth:

[15:07:12] ERROR - 002003 (02000): SQL compilation error:
Database 'XXX' does not exist or not authorized.

indicating successful authentication

@jakedave
Copy link
Author

jakedave commented Mar 22, 2023

Looks like without dbt, key is expected to be a file always; in dbt, the name is private_key_path for a file. Easy to switch, but the naming would feel incorrect; key feels like it should be the string version, not the file version. Please advise on naming convention

I have a local commit waiting for this PR which renames DBT's private_key, private_key_path, and private_key_passphrase to follow the current conventions here / whatever is decided and also removes the, now, duplicated code

@dlawin
Copy link
Contributor

dlawin commented Mar 22, 2023

key is expected to be a file always
key feels like it should be the string version, not the file version. Please advise

definitely agree here. What's unfortunate is this would introduce a breaking change for anyone currently using key as a path

@williebsweet interested in your thoughts here. This is a good long term change, but some users who are using the data-diff python api or --conf .toml file would need to change how they are designating a key for snowflake (they would just need to pass in key_path instead of key) after this makes it into a release

Co-authored-by: Dan Lawin <daniellawin@gmail.com>
@jakedave
Copy link
Author

Hi @williebsweet! Hoping to hear an update from you! We are excited to evaluate data-diff and require this change to be made in order to do that :)

@williebsweet
Copy link
Contributor

Hi @williebsweet! Hoping to hear an update from you! We are excited to evaluate data-diff and require this change to be made in order to do that :)

@jakedave Thanks for bringing this back to my attention. I'm working with Dan on a comms strategy for the breaking change so it might take a bit to get this merged and released.

To get your evaluation underway - have you tried pip installing the branch that you introduced the changes on? While it's not ideal, you'd at least get to try out the changes that you've made.

@jakedave
Copy link
Author

Thanks for the reply! Yes; to be clear, it's a blocker to evaluation beyond my current local environment (I.e. wider team adoption). Happy to hear that conversations are ongoing and looking forward to hearing more

encryption_algorithm=serialization.NoEncryption(),
)

if "key" in kw or "key_path" in kw:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jakedave I'm just realizing that dbt only supports a private_key_path, there's no need to add any support for a string

Are you able to rework this with the previous assumption? kw.get("key") <--- path

We shouldn't stray from what is defined here https://docs.getdbt.com/reference/warehouse-setups/snowflake-setup#key-pair-authentication

Copy link
Contributor

Choose a reason for hiding this comment

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

Once that's done we can release a new sqeleton version, add the new sqeleton version in the data-diff PR, and remove this method there

    @staticmethod
    def _get_snowflake_private_key(credentials):

@dlawin
Copy link
Contributor

dlawin commented Mar 31, 2023

Hey Jake, we want to get this in the next release (soon) so I ran with a few PRs of my own:
datafold/data-diff#468
#34
datafold/data-diff#469

Appreciate your contributions here!

@dlawin dlawin closed this Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants