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: code reorganization #285

Merged

Conversation

zanebclark
Copy link

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
Copy link
Author

zanebclark commented Sep 26, 2024

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

@zanebclark
Copy link
Author

@MACKAT05 , fyi

@sanelson
Copy link

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
Copy link
Author

@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
Copy link
Collaborator

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
Copy link
Author

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

@jamesweakley jamesweakley merged commit b5ba046 into Snowflake-Labs:master Oct 7, 2024
1 check passed
@zanebclark zanebclark deleted the feature/code-reorganization branch October 8, 2024 14:47
@sfc-gh-jhansen
Copy link
Collaborator

sfc-gh-jhansen commented Oct 17, 2024

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.]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants