diff --git a/morpc/__init__.py b/morpc/__init__.py index f00bb09..42bdad4 100644 --- a/morpc/__init__.py +++ b/morpc/__init__.py @@ -1,4 +1,4 @@ -__version__ = "0.7.0" +__version__ = "0.7.1" import logging logger = logging.getLogger(__name__) diff --git a/morpc/frictionless/release.py b/morpc/frictionless/release.py index 7a56b3f..a6d785e 100644 --- a/morpc/frictionless/release.py +++ b/morpc/frictionless/release.py @@ -343,7 +343,98 @@ def _batch_assets(assets, baseLength, limit=30000): return batches -def create_release(resources, owner, repo, tag, title=None, notes=None, overrideNotes=None, assets=None, dryRun=False): +def _git(args, cwd): + """Run a git command in cwd and return (returncode, stdout stripped of trailing whitespace).""" + import subprocess + + result = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) + return result.returncode, result.stdout.strip() + + +def _check_worktree_synced(dir, dryRun=False): + """Check that dir's repository is clean and in sync with its remote, raising if it is not. + + `gh release create` with no --target cuts the tag at the default branch's HEAD *on GitHub*. Work + that has not reached GitHub is therefore not in the release, even though the descriptors uploaded + as assets were built from it. The common way to hit this is to run a notebook end to end: the + rebuilt descriptors are still only in the working tree when the release cell runs, so the tag + lands on the previous build's commit. + + Three states produce that outcome, and all three are checked: uncommitted changes, commits that + have not been pushed, and a current branch that is not the one the tag will be cut on. + + The ahead/behind comparison is made against the last-fetched state of the remote ref. Nothing is + fetched here, so a remote commit made since the last fetch is not seen. That does not affect the + case this guards against, which is local work that has not gone out. + + Parameters + ---------- + dir : str or PathLike + A directory inside the repository to check. A falsy value means the working directory. + dryRun : bool + Optional. If True, log a warning instead of raising. A dry run is what one does mid-build + with a dirty tree, so failing it would make it useless. Defaults to False. + + Raises + ------ + RuntimeError + If the repository is dirty, out of sync, or not a repository at all, and dryRun is False. + """ + import shutil + + dir = dir or "." + + def fail(message): + if dryRun: + logger.warning("{} Not raising because this is a dry run.".format(message)) + return True + logger.error(message) + raise RuntimeError(message) + + if shutil.which("git") is None: + return fail("git is required to check that the release will include your work but was not found on PATH.") + + code, _ = _git(["rev-parse", "--show-toplevel"], dir) + if code != 0: + return fail("{} is not inside a git repository, so there is no way to check that the release will include your work.".format(dir)) + + code, status = _git(["status", "--porcelain"], dir) + if code != 0: + return fail("Could not read the status of the repository at {}.".format(dir)) + if status: + paths = "\n".join(" {}".format(line) for line in status.splitlines()) + return fail("The working tree has uncommitted changes. The release tag is cut at the branch head on GitHub, so these would not be in the release:\n{}\nCommit and push them, then create the release.".format(paths)) + + code, upstream = _git(["rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}"], dir) + if code != 0: + return fail("The current branch has no upstream branch, so there is no way to check that your commits have reached GitHub. Push it first.") + + code, counts = _git(["rev-list", "--left-right", "--count", "{}...HEAD".format(upstream)], dir) + if code != 0: + return fail("Could not compare the current branch with {}.".format(upstream)) + behind, ahead = (int(count) for count in counts.split()) + if ahead: + return fail("The current branch is {} commit(s) ahead of {}. The release tag is cut at the branch head on GitHub, so those commits would not be in the release. Push them, then create the release.".format(ahead, upstream)) + if behind: + return fail("The current branch is {} commit(s) behind {}. The release would be cut at commits you have not seen. Pull, re-run the build, then create the release.".format(behind, upstream)) + + # The remote's own HEAD names the branch the tag will be cut on. Many clones never set it, in + # which case the branch cannot be checked and the two checks above still stand on their own. + remote = upstream.split("/")[0] + code, defaultRef = _git(["symbolic-ref", "--short", "refs/remotes/{}/HEAD".format(remote)], dir) + if code != 0: + logger.warning("{}/HEAD is not set locally, so the current branch cannot be checked against the default branch. Run `git remote set-head {} -a` to set it.".format(remote, remote)) + return + + defaultBranch = defaultRef.split("/", 1)[1] + code, branch = _git(["rev-parse", "--abbrev-ref", "HEAD"], dir) + if code != 0: + return fail("Could not determine the current branch of the repository at {}.".format(dir)) + if branch != defaultBranch: + return fail("The current branch is {}, but the release tag is cut at {}, the default branch on GitHub. Merge {} into {} and push, then create the release.".format(branch, defaultBranch, branch, defaultBranch)) + + +def create_release(resources, owner, repo, tag, title=None, notes=None, overrideNotes=None, assets=None, allowDirty=False, dryRun=False): """Create a GitHub release from a set of published resource descriptors. This is the multi-resource counterpart to the hand-rolled `gh release create` step a workflow @@ -354,6 +445,10 @@ def create_release(resources, owner, repo, tag, title=None, notes=None, override in memory may have no known descriptor path, and the descriptor file is itself an asset that must be uploaded. Use prepare_release() to publish descriptors first; pass the same paths here. + The tag is cut at the default branch's head on GitHub, not at the local HEAD, so the repository + must be clean and pushed before the release is created. That is checked first, and nothing is + published if the check fails -- see _check_worktree_synced(). + The release is created without assets, which are then uploaded in batches small enough to fit within the maximum command line length. If an upload fails, the release and its tag are deleted so that the release can be retried once the cause is fixed, rather than left partially populated. @@ -381,10 +476,15 @@ def create_release(resources, owner, repo, tag, title=None, notes=None, override assets : list of str Optional. Extra asset paths to upload alongside the ones derived from resources, e.g. a data package descriptor. + allowDirty : bool + Optional. If True, skip the check that the repository is clean and in sync with its remote. + The release may then be cut at a commit that does not contain the data it describes. Defaults + to False. dryRun : bool Optional. If True, run every preflight check except the tag-existence check, log the resolved asset list and notes, and return without creating a release or calling `gh` to check the tag. - Defaults to False. + The repository check logs a warning rather than raising, since a dry run is what one does + mid-build with a dirty tree. Defaults to False. Returns ------- @@ -402,6 +502,11 @@ def create_release(resources, owner, repo, tag, title=None, notes=None, override if isinstance(resources, str): resources = [resources] + if allowDirty: + logger.warning("allowDirty is set. Not checking whether the release will include the data it describes.") + else: + _check_worktree_synced(os.path.dirname(resources[0]), dryRun=dryRun) + descriptors = [] assetPaths = [] for resourcePath in resources: diff --git a/reference/dev_notes.md b/reference/dev_notes.md index 03ca654..b67aae5 100644 --- a/reference/dev_notes.md +++ b/reference/dev_notes.md @@ -68,3 +68,32 @@ command line length on the paths alone and fail the same way (WinError 206). release exists with only some assets. The rollback narrows that window but cannot close it: if the delete also fails, a partial release and its tag are left behind and must be removed by hand before retrying. + +## 2026-09-14 — Check the repository is clean and pushed before creating a release + +Branch: `feat/release-worktree-guard` + +`gh release create` with no `--target` cuts the tag at the default branch's HEAD *on +GitHub*, not at the local HEAD. Running a notebook end to end therefore produced a +release whose tag predated the build it describes: the rebuilt descriptors, the HTML +export and the metadata were still only in the working tree when the release cell ran. + +- Added `_check_worktree_synced(dir, dryRun=False)`, called first in `create_release`. + It raises if the directory is not in a repository, if the working tree is dirty + (untracked-but-not-ignored files included), if the branch has no upstream, if it is + ahead of or behind that upstream, or if it is not the remote's default branch. The + dirty-tree message lists the offending paths. +- `dryRun=True` downgrades all of these to a warning, since a dry run is what one does + mid-build with a dirty tree. +- Added an `allowDirty` parameter as an escape hatch. It warns when set. +- Nothing is fetched, so the ahead/behind comparison is against the last-fetched state + of the remote ref. That does not affect the case this guards against, which is local + work that has not gone out. +- If `/HEAD` is not set locally (many clones never set it) the default-branch + check is skipped with a warning naming `git remote set-head`; the other checks stand. + +**Consequence for workflow repos:** the HTML export must now run *before* the release +cell and be committed with everything else, so the rendered run will not show the output +of the release cell itself. Repos that track files rewritten during a run — a `*.log` +that is not gitignored, notably `morpc-parcels-standardize` — cannot cut a release at +all until those are ignored. diff --git a/tests/test_release.py b/tests/test_release.py index 654f6d1..4eab2ed 100644 --- a/tests/test_release.py +++ b/tests/test_release.py @@ -23,7 +23,20 @@ resolve_data_path, write_resource, ) -from morpc.frictionless.release import _batch_assets +from morpc.frictionless import release +from morpc.frictionless.release import _batch_assets, _check_worktree_synced + + +@pytest.fixture(autouse=True) +def _skip_worktree_check(monkeypatch): + """Neutralize create_release()'s repository preflight for the tests that are not about it. + + Those tests build resources in tmp_path, which is not a repository, so the preflight would fail + them all for a reason none of them is testing. It is exercised directly by its own tests below, + which call _check_worktree_synced through the name imported above -- that name is bound to the + real function and is unaffected by patching the module attribute here. + """ + monkeypatch.setattr(release, "_check_worktree_synced", lambda *args, **kwargs: None) ASSET_URL = "https://github.com/morpc/morpc-parcels-standardize/releases/download/v2026.7.22/data.csv" @@ -1258,3 +1271,179 @@ def test_load_data_local_resource_is_unaffected(tmp_path): ) data, resource, schema = load_data(str(resourcePath)) assert data["id"].tolist() == [1, 2] + + +# --- _check_worktree_synced --- + +def _run_git(*args, cwd): + subprocess.run(["git", *args], cwd=str(cwd), check=True, capture_output=True) + + +def _make_repo(tmp_path, name="repo"): + """A repo with one commit, pushed to a bare remote whose HEAD names the default branch.""" + remote = tmp_path / "{}.git".format(name) + repo = tmp_path / name + subprocess.run(["git", "init", "--bare", "-b", "main", str(remote)], check=True, capture_output=True) + subprocess.run(["git", "init", "-b", "main", str(repo)], check=True, capture_output=True) + _run_git("config", "user.email", "test@example.com", cwd=repo) + _run_git("config", "user.name", "Test", cwd=repo) + (repo / "README.md").write_text("hello\n") + _run_git("add", "-A", cwd=repo) + _run_git("commit", "-m", "initial", cwd=repo) + _run_git("remote", "add", "origin", str(remote), cwd=repo) + _run_git("push", "-u", "origin", "main", cwd=repo) + _run_git("remote", "set-head", "origin", "-a", cwd=repo) + return repo + + +def test_check_worktree_synced_passes_when_clean_and_pushed(tmp_path): + repo = _make_repo(tmp_path) + + assert _check_worktree_synced(str(repo)) is None + + +def test_check_worktree_synced_modified_file_raises_and_names_it(tmp_path): + repo = _make_repo(tmp_path) + (repo / "README.md").write_text("changed\n") + + with pytest.raises(RuntimeError, match="README.md"): + _check_worktree_synced(str(repo)) + + +def test_check_worktree_synced_untracked_file_raises(tmp_path): + repo = _make_repo(tmp_path) + (repo / "data.resource.yaml").write_text("name: data\n") + + with pytest.raises(RuntimeError, match="data.resource.yaml"): + _check_worktree_synced(str(repo)) + + +def test_check_worktree_synced_ignored_file_is_not_dirty(tmp_path): + repo = _make_repo(tmp_path) + (repo / ".gitignore").write_text("*.log\n") + _run_git("add", "-A", cwd=repo) + _run_git("commit", "-m", "ignore logs", cwd=repo) + _run_git("push", cwd=repo) + (repo / "run.log").write_text("a log line\n") + + assert _check_worktree_synced(str(repo)) is None + + +def test_check_worktree_synced_dirty_tree_only_warns_on_a_dry_run(tmp_path, caplog): + repo = _make_repo(tmp_path) + (repo / "README.md").write_text("changed\n") + + with caplog.at_level("WARNING"): + assert _check_worktree_synced(str(repo), dryRun=True) is True + assert "README.md" in caplog.text + + +def test_check_worktree_synced_unpushed_commit_raises(tmp_path): + repo = _make_repo(tmp_path) + (repo / "README.md").write_text("changed\n") + _run_git("commit", "-am", "a commit that never left", cwd=repo) + + with pytest.raises(RuntimeError, match="ahead"): + _check_worktree_synced(str(repo)) + + +def test_check_worktree_synced_behind_the_remote_raises(tmp_path): + repo = _make_repo(tmp_path) + other = tmp_path / "other" + subprocess.run(["git", "clone", str(tmp_path / "repo.git"), str(other)], check=True, capture_output=True) + _run_git("config", "user.email", "test@example.com", cwd=other) + _run_git("config", "user.name", "Test", cwd=other) + (other / "README.md").write_text("someone else's change\n") + _run_git("commit", "-am", "upstream moved", cwd=other) + _run_git("push", cwd=other) + _run_git("fetch", cwd=repo) + + with pytest.raises(RuntimeError, match="behind"): + _check_worktree_synced(str(repo)) + + +def test_check_worktree_synced_branch_without_an_upstream_raises(tmp_path): + repo = _make_repo(tmp_path) + _run_git("checkout", "-b", "feature", cwd=repo) + + with pytest.raises(RuntimeError, match="no upstream"): + _check_worktree_synced(str(repo)) + + +def test_check_worktree_synced_non_default_branch_raises(tmp_path): + repo = _make_repo(tmp_path) + _run_git("checkout", "-b", "feature", cwd=repo) + _run_git("push", "-u", "origin", "feature", cwd=repo) + + with pytest.raises(RuntimeError, match="default branch"): + _check_worktree_synced(str(repo)) + + +def test_check_worktree_synced_skips_the_branch_check_when_remote_head_is_unset(tmp_path, caplog): + repo = _make_repo(tmp_path) + _run_git("symbolic-ref", "-d", "refs/remotes/origin/HEAD", cwd=repo) + _run_git("checkout", "-b", "feature", cwd=repo) + _run_git("push", "-u", "origin", "feature", cwd=repo) + + with caplog.at_level("WARNING"): + assert _check_worktree_synced(str(repo)) is None + assert "origin/HEAD is not set" in caplog.text + + +def test_check_worktree_synced_outside_a_repository_raises(tmp_path): + with pytest.raises(RuntimeError, match="not inside a git repository"): + _check_worktree_synced(str(tmp_path)) + + +def test_check_worktree_synced_missing_git_raises(tmp_path, monkeypatch): + monkeypatch.setattr(shutil, "which", lambda name: None) + + with pytest.raises(RuntimeError, match="git is required"): + _check_worktree_synced(str(tmp_path)) + + +def test_create_release_dirty_worktree_raises_before_any_gh_call(tmp_path, monkeypatch): + # Opt out of the autouse stub: this test is about the preflight running inside create_release. + monkeypatch.setattr(release, "_check_worktree_synced", _check_worktree_synced) + monkeypatch.setattr(shutil, "which", lambda name: "/usr/bin/gh" if name == "gh" else "/usr/bin/git") + repo = _make_repo(tmp_path) + _build_data(repo) + resourcePath = repo / "data.resource.yaml" + create_resource("data.csv", resourcePath=str(resourcePath), ignoreSchema=True, name="parcels", writeResource=True) + + calls = [] + realRun = subprocess.run + + def _fake_run(args, **kwargs): + calls.append(args) + # The preflight shells out to git for real; only gh is faked. + if args[0] == "git": + return realRun(args, **kwargs) + return _FakeCompleted(1) + + monkeypatch.setattr(subprocess, "run", _fake_run) + + with pytest.raises(RuntimeError, match="uncommitted changes"): + create_release([str(resourcePath)], "morpc", "repo", "v2026.7.22") + + assert not [args for args in calls if args[0] == "gh"] + + +def test_create_release_allow_dirty_skips_the_check(tmp_path, monkeypatch): + monkeypatch.setattr(release, "_check_worktree_synced", _check_worktree_synced) + monkeypatch.setattr(shutil, "which", lambda name: "/usr/bin/gh") + _build_data(tmp_path) + resourcePath = tmp_path / "data.resource.yaml" + create_resource("data.csv", resourcePath=str(resourcePath), ignoreSchema=True, name="parcels", writeResource=True) + + calls = [] + + def _fake_run(args, **kwargs): + calls.append(args) + return _FakeCompleted(1) + + monkeypatch.setattr(subprocess, "run", _fake_run) + + create_release([str(resourcePath)], "morpc", "repo", "v2026.7.22", allowDirty=True) + + assert calls[0][:3] == ["gh", "release", "view"]