feat(cli): per-resource error isolation + non-zero exit on failure for resubmit/submit#299
Merged
Conversation
…r resubmit/submit Before this, ``ckan datapusher_plus resubmit`` was a 16-line loop with: - no try/except — a single resource raising during ``datapusher_submit`` killed the whole batch and lost all later resources; - no end-of-run summary — operators had to grep the stream of "OK" / "Fail" lines to count failures; - unconditional exit 0 — a fully-failed batch was indistinguishable from a fully-succeeded one from a shell-exit / CI / cron perspective. ``_submit`` now isolates per-resource exceptions, classifies each resource into one of ``ok`` / ``fail`` / ``error``, prints a summary block at the end, and returns a bool the commands use to raise ``click.exceptions.Exit(code=1)`` when anything didn't succeed. A shared ``--continue-on-error/--stop-on-error`` flag is wired into both ``resubmit`` and ``submit``. Default is the legacy "drain the list" behaviour; ``--stop-on-error`` is for CI users who want to bail at the first failure (the ``submit`` command also stops processing further packages in that case). Out of scope (intentional): - No retry/backoff inside the CLI — the Prefect pipeline already has ``_retry_if_transient`` for the in-flow retries; the CLI's job is to surface outcomes, not to re-run the pipeline. - No ``--retry-failed`` state file — adds friction; can be a follow-up if it's actually wanted. Verify - 11 new unit tests in ``tests/test_cli_resubmit.py``: bucket bookkeeping, exception isolation, ``--stop-on-error`` semantics, summary rendering, and CLI exit codes via Click's CliRunner. - Full unit suite: 127/127 (was 116; +11 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Convert the three "Manual" checkboxes from PR #299 into automated ``INTEGRATION=1`` tests so future PRs verify the resubmit / submit CLI end-to-end and not just via CliRunner. These add the coverage CliRunner can't reach: that ``ckan datapusher_plus resubmit`` actually registers (entry_points wiring), finds real datastore-resident resources via ``datastore_backend.get_all_resources_ids_in_datastore``, and propagates its exit code out of the CKAN bootstrap into the shell. Three tests, one per plan item: 1. ``test_resubmit_exits_one_when_resource_fails`` — seeds a happy-path CSV → resource → datastore, then patches the resource URL to a 404 so the next ingest fails. Calls ``ckan datapusher_plus resubmit -y`` and asserts exit 1 + summary mentions the failing resource id. 2. ``test_resubmit_stop_on_error_breaks_early`` — same setup as (1), invokes with ``--stop-on-error`` and asserts exit 1. Doesn't pin the ordering (datastore listing order isn't deterministic) but the unit suite already nails that down. 3. ``test_submit_good_package_exits_zero`` — happy-path ``ckan datapusher_plus submit <package> -y``; asserts exit 0 and ``Fail: 0`` / ``Error: 0`` in the summary. Invokes the CLI via ``docker compose exec ckan`` so the tests run on the host while the CKAN bootstrap lives inside the integration stack. Skips cleanly when ``docker`` or the compose file isn't available, so they don't break collection on machines without the stack set up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the ckan datapusher_plus resubmit / submit CLI operator experience by isolating per-resource failures, printing an end-of-run summary, and returning a non-zero exit code when any resource doesn’t succeed (with an optional --stop-on-error mode for CI).
Changes:
- Add shared
--continue-on-error/--stop-on-errorflag and makeresubmit/submitexit with code1when any resource fails/errors. - Refactor
_submit()to bucket outcomes into OK/Fail/Error, continue by default, and print a consolidated summary. - Add unit tests for
_submit()bookkeeping + CLI exit codes, plus integration tests that validate real container CLI exit-code propagation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
ckanext/datapusher_plus/cli.py |
Adds stop/continue behavior, per-resource error isolation, summary output, and non-zero exit status on any non-OK outcome. |
tests/test_cli_resubmit.py |
New unit tests validating _submit() bucketing, stop-on-error semantics, summary rendering, and Click exit codes. |
tests/integration/test_cli_resubmit.py |
New integration tests running the real ckan CLI in Docker to assert registration + exit-code propagation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Four small follow-ups from Copilot's inline review:
* cli.py: add ``else: Context = dict`` fallback under the
``tk.check_ckan_version("2.10")`` guard so ``cast(Context, ...)`` in
``submit`` / ``_submit`` doesn't NameError on older CKANs. ``cast``
is a runtime no-op (returns its second arg unchanged), so ``dict``
is a safe stand-in while keeping the 2.10+ type-checker signal.
* cli.py: drop "datastore" from the ``_submit`` progress header.
``_submit`` is shared between ``resubmit`` (datastore-resident
resources) and ``submit`` (package resources, possibly pre-datastore),
so "Submitting N resource(s)" matches both call sites.
* tests/test_cli_resubmit.py: reword the module docstring. The
pre-refactor ``_submit`` didn't "swallow every exception" — it had
no try/except, so the first raising resource crashed the whole
batch (Click surfaced the traceback). When nothing raised it still
always exited 0; that part was right.
* tests/integration/test_cli_resubmit.py: make
``_docker_compose_available()`` actually probe the Compose v2 plugin
via ``docker compose version`` (instead of just checking for the
``docker`` binary). Systems with only Compose v1 / ``docker-compose``
now skip cleanly instead of failing inside ``_run_ckan_cli``.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 17, 2026
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
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.
Summary
Before this,
ckan datapusher_plus resubmitwas a 16-line loop with three pretty serious operator-experience holes:datapusher_submit(e.g. a transient network blip) killed the whole batch and lost every resource queued after it.OK/Faillines to count failures.exit 0— a fully-failed batch was indistinguishable from a fully-succeeded one from a shell-exit / CI / cron perspective. Impossible to alert on.After this PR:
ok/fail(returned falsy) /error(raised).resubmit/submitraiseclick.exceptions.Exit(code=1)when anything didn't succeed → CI gets the failure signal.--continue-on-error/--stop-on-errorflag on both commands. Default is the legacy "drain the list" behaviour;--stop-on-erroris for CI users who want to bail at the first failure. Insubmit,--stop-on-erroralso stops processing further packages.What's intentionally out of scope
_retry_if_transientfor in-flow retries; the CLI's job is to surface outcomes, not to re-run the pipeline.--retry-failedstate file — adds friction; can be a follow-up if anyone actually wants it.Note: this is unrelated to PR #253's "made DP+ resubmit more robust" commit — that one was about job-level rollback in the now-deleted
jobs.py, which is closer to issue #265 (data-dictionary stash) territory. The CLI-level work here is its own thing.Test plan
tests/test_cli_resubmit.py:_submitbucket bookkeeping for OK / Fail / Error mixes--stop-on-errorsemantics (break on first Fail, break on first Error)Skippedonly when stopped early)click.testing.CliRunner(exit 0 vs exit 1 vs--stop-on-errorearly termination)ckan datapusher_plus resubmit -yon a small dataset with one failing resource — verifyFail: 1summary + exit 1.ckan datapusher_plus resubmit -y --stop-on-error— verify it stops at the first failure.ckan datapusher_plus submit <good-package> -y— verify exit 0.CLI surface (new)
🤖 Generated with Claude Code