-
Notifications
You must be signed in to change notification settings - Fork 31
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
Modernize giftless #125
Modernize giftless #125
Conversation
So, this is a pretty good start, I think, at a modernized giftless. It now requires Python 3.10 or greater to run, but all its dependencies are up to date. The major thing that needs attention, and I think it probably needs attention from the Datopian side, is that moving to a newer set of Google libraries broke the Google Storage testing suite, which is extremely opaque (being pytest-vcr cassettes recorded against some Datopian set of runs against their repos). Hopefully someone at Datopian knows how these were produced and can just recreate them using the new code as a base. I'm confident that Google still works as a storage backend, although I have only tested it with Workload Identity credentials (since that's what I use) rather than stored keys. But it's working fine for my Git-LFS implementation. The other thing is that I took out pytest-flake8, so right now there's no test that the codebase passes flake8 (I ran it by hand to make sure it still does, and I feel like this/isort/tox all ought to be pre-commit hooks too). pytest-flake8 is unmaintained and doesn't work with recent flake8s, and we should probably switch over to using ruff or black rather than flake8. That's work I didn't want to tackle right now. I've got some pre-commit hooks for other Rubin Observatory projects that can probably be repurposed here pretty easily to get us back up to snuff, and add ruff/black to the set of checks as well. |
@athornton is this good to merge as is or do we need to wait on the other points you mention?
I'm wondering if should try redoing this testsuite in some more standardized way. (plus the original author of those tests is no longer with us!)
Let's replace flake8 with black. Would you be up for that? And thank-you again for the excellent well-explained contributions 🙏🚀 |
I think merging it as-is is better than not merging it, in that it's getting increasingly hard to install Python 3.7 since it's past EOL, and therefore it's been removed from brew and stuff like that. And the test suite...well... I very very much agree that a test suite that mocks out the cloud providers' responses and doesn't rely on the opaque wire-traffic captures that are basically what pytest-vcr does, would be a lot more maintainable and usable going forwards...but that's going to be quite a bit of work. It's certainly worth doing, and there may already be useful testing libraries for AWS and GCP--we're not the first people to write services against them, after all. I don't know HOW much work, though, which worries me. When I'm back at work (next week, assuming I'm not firefighting) I'll freshen up the github actions and tox with basically-what-we-use-in-the-Rubin-project for linting and style-guide formatting. It's not very different than what the flake8 here did, although we tend to use shorter lines....but black will handle that for us. That's going to be one very large and very boring commit, so I'll make sure that "apply new formatting standards" is its own commit. |
@athornton great. Ok i'll merge this for now. Also LMK if we should move (some parts) of this discussion to a general issue about "upgrading giftless" so it doesn't get lost across different PRs 🙂 |
A general-purpose issue seems like a good idea to me. I'll list the couple of things that immediately come to mind:
...when I'm back at my work computer next week I'll see if more occurs. |
Ok created #126 |
Build with 3.10 as a base and move all dependencies to their current versions.