Skip to content

Fix setup apply failure exit status#5098

Open
mattfaltyn wants to merge 5 commits into
apache:mainfrom
mattfaltyn:agent/fix-setup-apply-exit-code
Open

Fix setup apply failure exit status#5098
mattfaltyn wants to merge 5 commits into
apache:mainfrom
mattfaltyn:agent/fix-setup-apply-exit-code

Conversation

@mattfaltyn

@mattfaltyn mattfaltyn commented Jul 19, 2026

Copy link
Copy Markdown

Summary

  • Track terminal setup failures, including discovery/list operations, during setup apply.
  • Preserve best-effort processing so later resources are still attempted.
  • Exit with the standard CLI error status instead of logging a false success.

Why

setup apply catches setup API exceptions to continue applying the rest of a
configuration. Previously, those caught failures were never reflected in the
command result, so automation received exit status 0 and a success message even
when one or more requested resources were not provisioned. Discovery failures
could also be cached as empty results, causing later work to be skipped without
failing the command.

The command now counts those failures and raises a CliError after processing
the configuration. Regression tests verify the nonzero result, best-effort
continuation, discovery-failure handling (including policy lookups), invalid
external-catalog configuration, unresolved role assignments, and the expected
dry-run 404 behavior.

Fixes #5097

Testing

  • make client-unit-test (159 tests passed on Python 3.14.6)
  • PRE_COMMIT_HOME=/tmp/polaris-pre-commit make client-lint (all format, Ruff, and mypy hooks passed)
  • make client-license-check (passed)
  • JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@21/bin:$PATH ./gradlew format compileAll (build successful)
  • JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@21/bin:$PATH ./gradlew check (attempted; unrelated Docker-backed polaris-admin and Azurite tests could not start because Docker is not installed locally)
  • The exact CI Gradle command (check sourceTarball distTar distZip publishToMavenLocal -x :polaris-runtime-service:test -x :polaris-admin:test -PnoIntegrationTests --continue) completed all 1,887 tasks except the Docker-backed :polaris-relational-jdbc:test, whose Testcontainers PostgreSQL initialization could not start without a local Docker socket.

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes Python CLI setup apply exits successfully after provisioning failures #5097
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic (no complex logic added)
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (not needed; CLI syntax and configuration are unchanged)

@MonkeyCanCode

Copy link
Copy Markdown
Contributor

Thanks for the fix @mattfaltyn. The added code is counting failures for creation but not for listing. I think we should do that as well as we have couple of the _get_existing_* functions.

@mattfaltyn

Copy link
Copy Markdown
Author

Thanks for the fix @mattfaltyn. The added code is counting failures for creation but not for listing. I think we should do that as well as we have couple of the _get_existing_* functions.

Listing failures are counted now too, and I added tests. Thanks for catching that.

@MonkeyCanCode

Copy link
Copy Markdown
Contributor

Thanks for the fix @mattfaltyn. The added code is counting failures for creation but not for listing. I think we should do that as well as we have couple of the _get_existing_* functions.

Listing failures are counted now too, and I added tests. Thanks for catching that.

Thanks for the quick fix. I will take a look later tonight.

MonkeyCanCode
MonkeyCanCode previously approved these changes Jul 20, 2026
def execute(self, api: PolarisDefaultApi) -> None:
"""Execute the setup command based on the subcommand (apply or export)."""
if self.setup_subcommand == Subcommands.APPLY:
self._failure_count = 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is not necessary right as we default to 0 in the filed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Each CLI and REPL invocation creates a new SetupCommand, so the field default is sufficient. Removed the redundant reset in ae652a8.

@github-project-automation github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 20, 2026
@MonkeyCanCode

Copy link
Copy Markdown
Contributor

@mattfaltyn LGTM. Thanks for the fix. Left one minor nit.

except Exception:
logger.warning(
f"Could not verify existence of policy '{policy_name}', attempting creation."
self._record_failure(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could have a false failure when a policy existence check fails but creation then succeeds.
The generic exception branch of the load_policy check now calls _record_failure() while still setting policy_exits = false and proceeding to create the policy. If that load_policy error was transient and the subsequent create_policy succeeds, the resource was provisioned correctly yet the whole command still exits non-zero. Every other _record_failure() marks an operation that actually failed, but this one marks the operation that may well have succeeded.

I suggest to consider only recording the failure if the follow-up create also fails (or back to a warning).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. In ae652a8, a real apply now logs the lookup failure as a warning and lets the follow-up create determine the result. A successful create no longer increments the failure count, while dry-run still records the lookup failure because no create occurs. I also added a regression test for the recovered path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python CLI setup apply exits successfully after provisioning failures

3 participants