-
Notifications
You must be signed in to change notification settings - Fork 4
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
refactor: Fix linting, testing, CI, remove old backends and logs #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 couple of minor nits, but this this is good to merge once they're addressed or dismissed.
Thank you for culling out the unneeded mongo and citus backends, as well as all of those log files! Leaner and meaner tool now..
- I tested this by running this branch in my Aspects shell, on both the ralph and clickhouse backends.
- I read through the code
-
I checked for accessibility issuesN/A - Includes documentation
- Commit structure follows OEP-0051
@@ -29,6 +32,8 @@ | |||
license="AGPLv3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the install_requires
for pymongo[srv]
and psycopg2-binary
too?
Alternatively, borrow this: https://github.com/overhangio/tutor/blob/master/setup.py#L60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way Tutor does it has some issues with the OeX constraints and version pinning, but I removed those and matched the pinned versions in base.in. I also removed the numpy requirement since it seems like clickhouse-connect no longer throws an error when it's missing, and widened the constraint to <0.7
since it's been running 0.6.4
fine based off of the previous non-pinned versioning in setup.py. I'm not 100% sure this won't break things, but I also version bumped so we can test in the Aspects plugin and roll back if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested with a fresh environment
Long overdue cleanup and basic testing so we can be more confident about requirements updates. Manually tested against all 3 remaining backends successfully.