build: modernize forum repo to use uv, pyproject.toml, and semantic-release - #283
build: modernize forum repo to use uv, pyproject.toml, and semantic-release#283farhan wants to merge 2 commits into
Conversation
farhan
left a comment
There was a problem hiding this comment.
Review comments on the modernization PR — see individual inline threads for details.
farhan
left a comment
There was a problem hiding this comment.
Addressing inline comments — see individual threads for analysis and fixes.
5c5ba18 to
78f6e6f
Compare
34a2398 to
159830c
Compare
- Replace setup.cfg/setup.py with pyproject.toml (PEP 621/735) - Migrate to uv for dependency management, replacing pip-tools requirements files - Migrate to src/ layout - Add python-semantic-release for automated versioning and changelog - Update CI/CD workflows for uv and semantic-release - Update Makefile, docs, and tox config for new project structure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
701d1b4 to
7b56143
Compare
- Use uv in semantic_release build_command instead of bare pip
- Remove black from quality group (not in quality.in); belongs in ci only
- Add {include-group = "quality"} to ci group (mirroring ci.in -r quality.txt)
- Regenerate uv.lock
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
irfanuddinahmad
left a comment
There was a problem hiding this comment.
CI passes and the migration is otherwise careful (src-layout, entry points, coverage config, and the release workflow all line up correctly, and I confirmed the docs/package/e2e jobs are genuinely green on the PR). But there's a real, undisclosed regression in the quality gate:
Makefile: test-quality used to run test-lint test-codestyle test-mypy test-format. This PR drops test-format from that list:
-test-quality: test-lint test-codestyle test-mypy test-format ## run static coverage tests
+test-quality: test-lint test-codestyle test-mypy ## run static coverage teststest-quality is exactly what CI's quality tox env calls (make test-quality selfcheck), so black --check is no longer part of the CI quality gate — a formatting regression could now merge silently. This isn't mentioned anywhere in the PR description. Was this intentional, or a slip during the Makefile restructuring? If intentional, please call it out explicitly; if not, please restore it.
Minor, non-blocking notes:
- The PR body doesn't mention that
docswas added to the CI matrix or that Codecov integration is new — both are fine (I verifiedCODECOV_TOKENalready works org-wide, e.g. on the merged XBlock PR), but worth stating in the description since they're scope beyond "modernize tooling."
|
Thanks for catching this, @irfanuddinahmad — your point is valid. The removal of On I've opened issue #284 to track this properly. The plan is to replace |
irfanuddinahmad
left a comment
There was a problem hiding this comment.
CI is fully green (11/11) and the migration is careful — src-layout, entry points, coverage config, and the semantic-release/OIDC publish workflow all line up correctly. The test-format removal from test-quality is confirmed non-regressive (black was never enforced on master either, since it was missing from requirements/quality.txt) and is tracked with a concrete follow-up plan in #284. LGTM.
Important
PR implemented with the assistance of Claude Code. Refined and validated before being submitted for code review.
Modernize
forumPart of openedx/public-engineering#506
Summary
openedx-forumsrc/layoutsetup.py/setup.cfgwithpyproject.toml(PEP 621 static metadata)uvwith PEP 735 dependency groups; commituv.lockpyproject.tomlastral-sh/setup-uv; SHA-pin all actionspython-semantic-release+release.yml(OIDC trusted publishing)docsjob to CI matrix (builds Sphinx docs as part of every run)CODECOV_TOKENis already configured org-wide)Removed/Updated
Deleted files:
setup.py,setup.cfg,requirements/,.coveragerc,CHANGELOG.rst,mypy.ini,pypi-publish.ymlRemoved Makefile targets:
compile-requirementsuv lockpiptoolsuvhandles environment setupUpdated Makefile targets:
requirementsuv sync --group dev+uv tool install tox --with tox-uvupgradeedx_lint write_uv_constraints+uv lock --upgradeNotes on
test-formatremovaltest-format(which runsblack --check) was intentionally removed fromtest-quality. Investigation revealed that onmaster,blackis absent fromrequirements/quality.txt— the file thequalitytox env installs from — soblack --checkwas never actually executable in CI. The removal here maintains parity with whatmastertruly enforces rather than introducing a new failure.This is tracked in issue #284. The follow-up plan is to replace
blackwithruff format(the standard formatter for modernized Open edX repos), wire it into thequalitydependency group correctly, and restoretest-formatto the quality gate.Versioning
setuptools-scmwithdynamic = ["version"]— master had a PyPI publish workflow;python-semantic-releasecontrols the version string at release time via git tags.Important Notes
pypi-publish.ymlwas deleted becauserelease.ymlreplaces it with OIDC trusted publishing viapython-semantic-release; the old workflow used aPYPI_UPLOAD_TOKENsecret which is superseded by the OIDC approach.publish_to_pypijob will fail on the first release.Testing Notes
This PR has not been manually tested against the repo's own features. Testing relied on CI checks and local agent tooling (
make requirements,make lint,make test,python -m build). Repo-owner is encouraged to run the repo's feature tests before merging.🤖 Generated with Claude Code