search

Snowflake-Labs / schemachange

A Database Change Management tool for Snowflake
Apache License 2.0
482 stars 219 forks source link

Additional Authentication methods and Connection Refactor. #141

Closed MACKAT05 closed 1 year ago

MACKAT05 commented 1 year ago

Did a number of things here in addition to adding the authentication methods. The inversion of the authentication to a more top level part of the program removes a lot of overhead. I do have some concerns that could save some headaches when reviewing.

  1. I have been able to test the Oauth and external browser methods fairly easily however i don't have an environment to test Okta, I included it anyway since the specification in the snowflake connector seemed straightforward to implement.

  2. I included the additional Command line element exactly like the vars command.... however i could not successfully test it... --vars using the readme example produced a similar error on windows 'Invalid json loads.' so its not clear if the error was in what i was trying to feed it or just the windows environment. It would be nice to include this with nested data structures handled but some kind of prefixing mechanism could be used to group data payload and header data later if it must be in the command line as a flat structure. In addition the inclusion of secrets in the command line call also is a cause for concern and we may have to extricate it out.... and pull it from an environment variable later somehow....

  3. Its unclear whether the login per interaction to a singular login session would be a breaking change. depending on what end users are using for logging besides the inbuilt mechanism. There are 2 more transactions per script to update the query tag and instead of multiple sessions a single one....

  4. Internally I rewired a lot of things; a. I moved the messages and sql to variables so it would be easier to read and audit them. and for the ambitious users add localization. It also made it easier to keep the width under 100 or so characters.

b. I standardized on snake_case since python cannot handle kebab-case names as arguments; doing so allowed the use of dictionary unpacking to simplify the calls.

c. I also reworked some of the logic i touched to use a more flexible pattern. now when essential checks are updated its a matter of adding an item to a list.

Other minor issues: i could not get the markdown links to work from my editor.

MACKAT05 commented 1 year ago

There were a few elements from other pull requests i considered including but i was unsure of #132 and #139; i'll go add the dockerfile items now

MACKAT05 commented 1 year ago

Looks like i am also missing a test_SnowflakeSchemachangeSession.py

MACKAT05 commented 1 year ago

Looks like i am also missing a test_SnowflakeSchemachangeSession.py

I started to write these tests but i realized that it would be kind of hard to test the interaction without something ( as snowflake account / oauth provider) to talk to .... how should we handle this? we could add a 'Test' mode to the Class and just have the functions return the critical outputs?

sfc-gh-jhansen commented 1 year ago

Thanks @MACKAT05 (and @sfc-gh-tmathew), I'll take a look at this soon!

RavisundarDevops commented 1 year ago

Hi @MACKAT05 , @sfc-gh-jhansen , @sfc-gh-tmathew , Team, This has been a great feature addition to the tools, when can we expect this change to be live? Thanks in Advance

sfc-gh-jhansen commented 1 year ago

I will review on Monday!

sfc-gh-jhansen commented 1 year ago

Hey there @MACKAT05, I just merged these changes and noticed that the tests in the associated GitHub Action workflow failed. Can you please take a quick look to see why?

MACKAT05 commented 1 year ago

the default config needs to be updated to use snake case :/ missed that [image: image.png]

i'll check for similar issues and push soon

On Mon, Feb 6, 2023 at 11:42 AM Jeremiah Hansen @.***> wrote:

Hey there @MACKAT05 https://github.com/MACKAT05, I just merged these changes and noticed that the tests in the associated GitHub Action workflow failed. Can you please take a quick look to see why?

— Reply to this email directly, view it on GitHub https://github.com/Snowflake-Labs/schemachange/pull/141#issuecomment-1419646503, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC44T6OQU5ER4TD4DLYU2U3WWFH2LANCNFSM6AAAAAAUKO253A . You are receiving this because you were mentioned.Message ID: @.***>

MACKAT05 commented 1 year ago

@sfc-gh-jhansen updated test Files... is a new pull request needed?

sfc-gh-jhansen commented 1 year ago

Thanks @MACKAT05, and no need for a new pull request

sfc-gh-jhansen commented 1 year ago

Alright @MACKAT05, I'm not able to run the updated version of schemachange on my laptop with my test configuration that I had been using. I dug into it a bit and think I found another bug in this release. Previously all the metadata operations/functions which called execute_snowflake_query() passed as the first parameter the database of the change history table. And because that method would make a new connection to Snowflake each time (bad) it also set the default database each time. And we had two different default databases being used, one for metadata operations and one for the user scripts.

I like the changes you made to only connect once and want to keep that. But we need to figure out how best to specify the database for the metadata queries. Maybe the easiest thing we can do is just update the query templates to take a three part name and use all the parts of the change_history_table object?

MACKAT05 commented 1 year ago

if a script calls any of the permutations of "Use/Create database/schema/warehouse X" or "use role Y" we would have an issue since the state of the session would not return to configuration, and create unexpected behavior compared to legacy.
The relevant info is embedded in conargs... and is embedded in the class and can be accessed in the same code as the set tag functions. these can be expanded and renamed i'll push the update in a second.

MACKAT05 commented 1 year ago

I think that covers it... the metadata queries happen up front before any session changes occur and the ---ahhh i see... fixing that now...

MACKAT05 commented 1 year ago

I attempted to make adjustments to the tests to get them to pass... currently the 24 of them are complaining about the same issue that I think might be a config issue... since one looks like a linux path and the other a relative path... image the 25th i just fixed

MACKAT05 commented 1 year ago

sometimes all you need is a little sleep ; all tests passing now : image

sfc-gh-jhansen commented 1 year ago

Hey there @MACKAT05, I can't see any of your new changes here, so I guess we do need a new PR for this. I thought you could add to a previously merged one but it doesn't look like we can.

MACKAT05 commented 1 year ago

making it now :)

On Wed, Feb 8, 2023 at 12:13 PM Jeremiah Hansen @.***> wrote:

Hey there @MACKAT05 https://github.com/MACKAT05, I can't see any of your new changes here, so I guess we do need a new PR for this. I thought you could add to a previously merged one but it doesn't look like we can.

— Reply to this email directly, view it on GitHub https://github.com/Snowflake-Labs/schemachange/pull/141#issuecomment-1423182407, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC44T6P2S62FXL6AQPL6IL3WWP46ZANCNFSM6AAAAAAUKO253A . You are receiving this because you were mentioned.Message ID: @.***>