Closed pomegranited closed 1 year ago
Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR.
Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.
Please let us know once your PR is ready for our review and all tests are green.
Hi @ormsbee CC @bradenmacdonald :)
Are we ready to push version 0.1.0 of this package to pypi? This is needed by https://github.com/openedx/edx-platform/pull/32661
@pomegranited: Before you do this, could you please remove the lines I added around supporting the COMPRESSED row format and test? It turns out that AWS Aurora does not support COMPRESSED in its MySQL implementation.
Table creation should default to DYNAMIC
without that, which should be okay from the index size point of view. So just deleting it and the one line that calls it would be sufficient.
(I can also do this, but I wouldn't get around to it until tomorrow).
Sure thing @ormsbee , I've merged https://github.com/openedx/openedx-learning/pull/66
Anything else that we should fix before releasing this? Feel free to merge and watch the magic happen if we're ready :)
On a related note (non-blocking!), is there some Open edX policy of sticking with setup.py
and all the separate tooling files instead of a consolidated pyproject.toml
? I'm starting to see warnings from pip when installing packages like this that haven't migrated to pyproject.toml.
@bradenmacdonald: I think it's less of a policy issue and more of a "that's what cookiecutter spits out and updating things isn't high on the priority list". @jmbowman would likely know if there were any plans in that area, but I don't think there would be any objections if we started using pyproject.toml
?
Mainly we haven't gotten around to it yet (and aren't feeling a lot of pain around setup.py usage). There are a handful of repositories in both the edx
and openedx
orgs already using pyproject.toml, I'm totally up for a discovery ticket to figure out the best process to start switching things over. The one thing that might be at least a short term blocker is figuring out how to make OEP-18 work correctly without custom Python functions in setup.py.
I see, thanks for the info @ormsbee and @jmbowman
The one thing that might be at least a short term blocker is figuring out how to make OEP-18 work correctly without custom Python functions in setup.py.
While only the version
field supports actual python code to compute its value, the dependencies
field does allow you to load its value from a file which is in the requirements.txt
format. Though it says something about not supporting comments, which may be an issue.
Cool, thanks for your input here everyone!
I'm totally up for a discovery ticket to figure out the best process to start switching things over.
👍 to this -- happy to help trial the approach when you get a chance to plan one.
If there's nothing else that needs fixing here before we merge, do you want to do the honors @ormsbee ? 🚀
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.
Hmm.. merged, but no package run: https://github.com/openedx/openedx-learning/actions/workflows/pypi-publish.yml Maybe it only applies for PRs merged after this one?
@Agrendalath Do you happen to know why merging this didn't publish a pypi package? Do I need to create a release or something? cf FAL-3410
@Agrendalath Nevermind, I worked it out :) Just had to push a new tag, and it did it! https://pypi.org/project/openedx-learning/
Creates pypi-publish.yml using the procedure recommended by edx-platform.