Replace per-script Django bootstrap with a cli_dispatch stub#4144
Open
lunkwill42 wants to merge 3 commits into
Open
Replace per-script Django bootstrap with a cli_dispatch stub#4144lunkwill42 wants to merge 3 commits into
lunkwill42 wants to merge 3 commits into
Conversation
NAV predates Django, and many of its command line tools are standalone scripts that use Django's ORM without being management commands, so they need Django bootstrapped before anything of theirs that touches the ORM gets imported - the same problem manage.py solves for management commands. Until now this was done with a bare bootstrap_django(__file__) call sandwiched between two import blocks in each script, which blocks any future import-sorting enforcement. nav.cli_dispatch.main() bootstraps Django, then looks up and calls the real implementation via the new "nav.cli_commands" entry-point group, based on which command name the process was invoked as (available via sys.argv[0], which pip/setuptools-generated console scripts always set to the installed command name). Every Django-dependent command's [project.scripts] entry now points here instead of at its own module, with the real target moved to [project.entry-points."nav.cli_commands"] so the dispatcher can still find it - it can't read this back from [project.scripts] itself, since that now just says "nav.cli_dispatch:main" for these commands. Because bootstrapping happens before the implementing module is ever imported, that module's own top-level imports never need to work around Django not being configured yet - unlike a decorator applied to that module's own main(), which still requires anything the module uses at definition time (default argument values, decorators, class bodies) to already be Django-safe. The bin scripts themselves aren't touched by this commit yet: they still call bootstrap_django() on their own, which is now a redundant but harmless no-op (bootstrap_django() guards against being run more than once).
Several adaptations were needed for the test suite to correctly exercise scripts now routed through nav.cli_dispatch: - _nav_scripts_map() resolved a dispatched script's testarg-scanning target from [project.scripts], which now just says "nav.cli_dispatch:main" for these commands. It now prefers [project.entry-points."nav.cli_commands"], where the real implementing module lives. - test_script_runs spawned each script by inheriting the pytest process's full environment. Under a tox testenv, DJANGO_SETTINGS_MODULE is already exported there, so this never actually exercised bootstrap_django()'s own "var absent, set the default" branch. - naventity, navuser and smsd are dispatched via nav.cli_dispatch, so they must be invoked by their installed command name for dispatch to find the right target - a few tests invoked them by raw file path instead, which bypasses cli_dispatch (and installed-command discovery) entirely.
All of these are now dispatched via nav.cli_dispatch (see pyproject.toml's "nav.cli_commands" entry points), which bootstraps Django before any of these modules is ever imported. Calling bootstrap_django() here too was harmless (it's a no-op past the first call) but pointless, and kept these scripts' own imports from being ordinary, freely-sortable import blocks.
lunkwill42
added
the
nonews
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4144 +/- ##
==========================================
- Coverage 65.37% 65.32% -0.06%
==========================================
Files 629 630 +1
Lines 47206 47158 -48
==========================================
- Hits 30863 30808 -55
- Misses 16343 16350 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Test results 5 files 5 suites 7m 56s ⏱️ Results for commit 8f247d7. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope and purpose
NAV predates Django; its command line tools are standalone scripts that use Django's ORM without being Django management commands (effectively, NAV tries to insulate end users from Django). Until now, each of these ~30 scripts called
bootstrap_django(__file__)as a bare statement sandwiched between two import blocks, so that Django would be configured before the rest of the script's imports touched the ORM. That's a hack: it blocks any future import-sorting enforcement, and reads as one.This PR introduces
nav.cli_dispatch, a single console-script entry point that every Django-dependent command inpyproject.tomlnow points to instead of its own module. It bootstraps Django, then looks up and calls the real implementation via a new[project.entry-points."nav.cli_commands"]table, based on which command name the process was invoked as (available viasys.argv[0], which pip/setuptools-generated console scripts always set to the installed command name). Because bootstrapping now happens before the implementing module is ever imported, none of these ~30 scripts need to work around Django not being configured yet - their own top-level imports go back to being an ordinary, freely-sortable block.An alternative attempt at this used a decorator (
needs_django) applied to each script's ownmain()instead. That required real per-file workarounds wherever Django-dependent names were used outsidemain()itself:globalbuckets for names shared across several functions,None-sentinel default arguments for values that used to be computed from Django models at function-definition time,from __future__ import annotationsplusTYPE_CHECKINGguards for type hints, and relocatingnavoidverify's platform-specific epoll reactor install. None of that is needed here, since the whole module is only ever imported after Django is already configured.No user-facing behavior change. To observe the effect: any of the affected commands (e.g.
navdf,navuser list,navoidverify) should work exactly as before when run by their installed name;pytest tests/integration/bin_test.pyexercises all of them plus a couple of edge cases (naventity,navuser,smsd) that previously invoked their.pyfiles directly instead of via the installed command - see the second commit for why that had to change too.This could be a first stepping stone to finally introduce ruff-backed automatic import sorting across this codebase.
Contributor Checklist
Added a changelog fragment for towncrier(purely internal restructuring, no user-facing change - will add thenonewslabel instead)Added/changed documentation(bootstrap_django()itself is unchanged and still correctly used as-is bydoc/conf.pyanddoc/exts/alerttypes.py, which aren't part of this migration)master)Created new issues if this PR does not fix the issue completely(no further work is needed)If this results in changes in the UI: added screenshots(no UI change)nav/cli_dispatch.pyfile