feat: code reorganization

Snowflake-Labs / schemachange

A Database Change Management tool for Snowflake

Apache License 2.0

517 stars 229 forks source link

feat: code reorganization #285

Closed zanebclark closed 1 month ago

zanebclark commented 2 months ago

This is a resurrection of PR 222 and PR 275. This pull request contains a repo refactor with purposeful functionality changes.

Replace print statements and if verbose checks with a structlog logger and log levels.
Removed SecretManager in favor of a structlog processor that implements similar logic.
Centralize config-related logic and input checking into a Config object. For example, there was logic for checking the validity of a directory and the presence of environment variables. Centralizing this logic is conceptually easier for me to wrap my head around and makes testing much easier.
Generally, moved a bunch of class definitions out of cli.py and into separate files
Stored script details in a Script class to clarify the API
Removed a bunch of credential-related logic from SnowflakeClient and created a Credential factory.
Generally expanded test case coverage.

zanebclark commented 2 months ago

@sfc-gh-tmathew , this is the code reorganization pull request you asked for as part of 275

zanebclark commented 2 months ago

@MACKAT05 , fyi

sanelson commented 2 months ago

This is amazing work! I should have dug around in the old pull requests before submitting my own massive refactor pull request #282 a few weeks ago... If it will help this pull request get any traction, I'm happy to refactor all my changes on to these new changes proposed by @zanebclark . Seems like a really solid base to build off of. Nice work!

zanebclark commented 1 month ago

@sfc-gh-tmathew @jamesweakley @sfc-gh-jhansen, I'm getting some pressure to move away from my fork back to a repository-released version. What does the review process look like from here? Can I put any soft timelines in front of my client to push off the return to main?

jamesweakley commented 1 month ago

This looks like a nice refactor to me, appreciate the contribution.

Since @sfc-gh-tmathew has alredy provided feedback on the original PR, I'll wait for his review. I've set a reminder to come back early next week and check that it's been reviewed.

Since this is the "no functionality changes" PR it should be straightforward enough to approve.

zanebclark commented 1 month ago

Thanks, @jamesweakley. I'm standing on the shoulders of giants. Think about that next time you have back pain 😉

sfc-gh-jhansen commented 1 month ago

Hey there @zanebclark, thank you for the refactor, I agree that it looks helpful!

I'll let @sfc-gh-tmathew chime in here, but it looks like a few tests still aren't passing. See this GitHub Actions run: https://github.com/Snowflake-Labs/schemachange/actions/runs/11330550805. Can you please help us sort these out so we can get this released as version 4.0?

[And I apologize about the lack of a documented release process. We've been making some changes to it and are working on updated docs.]