Skip to content

Fix setup apply failure exit status #5098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mattfaltyn wants to merge 6 commits into
base: main
Choose a base branch
from
+253 −24
Open

Fix setup apply failure exit status #5098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1008267
Fix setup apply failure exit status
mattfaltyn Jul 19, 2026
e27a9f7
Count setup discovery failures
mattfaltyn Jul 19, 2026
8585e4a
Count remaining setup apply failures
mattfaltyn Jul 19, 2026
f4f1298
Count skipped setup role assignments
mattfaltyn Jul 19, 2026
74b5879
Recover successful policy creation after lookup errors
mattfaltyn Jul 20, 2026
b7964e5
Document and test successful setup apply
mattfaltyn Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- Deprecated `ALLOW_EXTERNAL_TABLE_LOCATION`. Use `ALLOW_EXTERNAL_METADATA_FILE_LOCATION` for external metadata file locations, including catalog config `polaris.config.allow.external.metadata.file.location`.

### Fixes
- Python CLI `setup apply` now exits with an error after any setup operation fails, while still attempting the remaining operations. Previously, individual failures were logged but the command reported success and exited with status 0.
- Python CLI `tables list`, `tables get`, and `tables delete` commands now exit with status 1 when catalog API requests fail. Previously, these commands printed an error but exited successfully.
- Default table storage locations with object-storage prefixing enabled are now percent-encoded with UTF-8 instead of the JVM default charset. Previously the persisted location of a table under a non-ASCII namespace or table name depended on the platform default charset, so the same table could resolve to a different (or lossy) location on a non-UTF-8 JVM.
- Async task execution (table cleanup, manifest and batch file cleanup) now retries when a handler returns false on transient errors (e.g. IO or delete failures). Previously `false` was swallowed with only a warning log and the task was never retried via the existing retry mechanism.
Expand Down
80 changes: 57 additions & 23 deletions client/python/apache_polaris/cli/command/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@
import logging
import yaml
import json
from dataclasses import dataclass
from dataclasses import dataclass, field
from typing import Dict, Optional, List, Any, Set

from apache_polaris.cli.command import Command
from apache_polaris.cli.exceptions import CliError
from apache_polaris.cli.exceptions import CliError, CLI_ERROR_EXIT_CODE
from apache_polaris.cli.constants import (
PrincipalType,
Subcommands,
Expand Down Expand Up @@ -76,6 +76,7 @@ class SetupCommand(Command):
_existing_catalogs: Optional[Set[str]] = None
_existing_principals: Optional[Set[str]] = None
_catalog_api: Optional[Any] = None
_failure_count: int = field(default=0, init=False, repr=False)

def _get_catalog_api(self, api: PolarisDefaultApi) -> Any:
"""Get or create and cache the IcebergCatalogAPI client."""
Expand All @@ -91,7 +92,7 @@ def _get_existing_principals(self, api: PolarisDefaultApi) -> Set[str]:
p.name for p in api.list_principals().principals
}
except Exception:
logger.exception("Failed to fetch existing principals")
self._record_failure("Failed to fetch existing principals")
self._existing_principals = set()
return self._existing_principals

Expand All @@ -103,7 +104,7 @@ def _get_existing_principal_roles(self, api: PolarisDefaultApi) -> Set[str]:
role.name for role in api.list_principal_roles().roles
}
except Exception:
logger.exception("Failed to fetch existing principal roles")
self._record_failure("Failed to fetch existing principal roles")
self._existing_principal_roles = set()
return self._existing_principal_roles

Expand All @@ -125,8 +126,8 @@ def _get_existing_catalog_roles(
# In dry-run, a 404 is expected if the catalog doesn't exist yet
is_404 = getattr(e, "status", None) == 404 or "(404)" in str(e)
if not (self.dry_run and is_404):
logger.warning(
f"Failed to fetch catalog roles for catalog '{catalog_name}': {e}"
self._record_failure(
f"Failed to fetch catalog roles for catalog '{catalog_name}'"
)
self._existing_catalog_roles[catalog_name] = set()
return self._existing_catalog_roles[catalog_name]
Expand All @@ -138,7 +139,7 @@ def _get_existing_catalogs(self, api: PolarisDefaultApi) -> Set[str]:
try:
self._existing_catalogs = {c.name for c in api.list_catalogs().catalogs}
except Exception:
logger.exception("Failed to fetch existing catalogs")
self._record_failure("Failed to fetch existing catalogs")
self._existing_catalogs = set()
return self._existing_catalogs

Expand Down Expand Up @@ -572,7 +573,12 @@ def execute(self, api: PolarisDefaultApi) -> None:
dry_run=self.dry_run,
)
logger.info(f"--- Finished processing catalog: {catalog_name} ---")
if self.dry_run:
if self._failure_count:
raise CliError(
f"Setup apply failed; setup errors: {self._failure_count}",
exit_code=CLI_ERROR_EXIT_CODE,
)
elif self.dry_run:
logger.info("=== Dry-Run Finished ===")
else:
logger.info("=== Setup Apply Process Completed Successfully ===")
Expand Down Expand Up @@ -601,6 +607,10 @@ def _log_dry_run(
message += f" with details:\n{details_str}"
logger.info(message)

def _record_failure(self, message: str) -> None:
self._failure_count += 1
logger.exception(message)

def _create_principals(
self,
api: PolarisDefaultApi,
Expand All @@ -609,6 +619,9 @@ def _create_principals(
) -> None:
"""Create principals and assign them to principal roles."""
logger.info("--- Processing principals ---")
if not principals_config:
logger.info("--- Finished processing principals ---")
return
existing_principals = self._get_existing_principals(api)
for principal_name, principal_data in principals_config.items():
if principal_name in existing_principals:
Expand Down Expand Up @@ -640,7 +653,7 @@ def _create_principals(
)
existing_principals.add(principal_name)
except Exception:
logger.exception(
self._record_failure(
f"Failed to create principal '{principal_name}'"
)
# Assign roles
Expand All @@ -657,6 +670,7 @@ def _create_principals(
logger.warning(
f"Skipping assignment for non-existing principal role '{role_name}' for principal '{principal_name}'"
)
self._failure_count += 1
continue

if dry_run:
Expand All @@ -681,7 +695,7 @@ def _create_principals(
f"Assigned principal '{principal_name}' to role '{role_name}' successfully."
)
except Exception:
logger.exception(
self._record_failure(
f"Failed to assign principal '{principal_name}' to role '{role_name}'"
)
logger.info("--- Finished processing principals ---")
Expand All @@ -694,6 +708,9 @@ def _create_principal_roles(
) -> None:
"""Create principal roles."""
logger.info("--- Processing principal roles ---")
if not principal_roles_config:
logger.info("--- Finished processing principal roles ---")
return
self._get_existing_principal_roles(api)

for role_name in principal_roles_config:
Expand Down Expand Up @@ -722,7 +739,9 @@ def _create_principal_roles(
if self._existing_principal_roles is not None:
self._existing_principal_roles.add(role_name)
except Exception:
logger.exception(f"Failed to create principal role '{role_name}'")
self._record_failure(
f"Failed to create principal role '{role_name}'"
)
logger.info("--- Finished processing principal roles ---")

def _map_storage_properties(self, catalog_data: Dict[str, Any]) -> Dict[str, Any]:
Expand Down Expand Up @@ -883,6 +902,7 @@ def _create_catalogs(
logger.warning(
f"External catalog '{catalog_name}' is missing the required 'connection' info block."
)
self._failure_count += 1
overall_success = False
continue
command_args.update(conn_args)
Expand Down Expand Up @@ -954,7 +974,7 @@ def _create_catalogs(
logger.info(f"Catalog '{catalog_name}' created successfully.")
existing_catalogs.add(catalog_name)
except Exception:
logger.exception(f"Failed to create catalog '{catalog_name}'")
self._record_failure(f"Failed to create catalog '{catalog_name}'")
overall_success = False
logger.info("--- Finished processing catalogs ---")
return overall_success
Expand All @@ -968,8 +988,14 @@ def _create_catalog_roles(
) -> None:
"""Create catalog roles, assign them to principal roles, and grant privileges."""
logger.info(f"--- Processing catalog roles for catalog: {catalog_name} ---")
if not roles_config:
logger.info(
f"--- Finished processing catalog roles for catalog: {catalog_name} ---"
)
return
existing_roles_in_catalog = self._get_existing_catalog_roles(api, catalog_name)
self._get_existing_principal_roles(api)
if any(role_data.get("assign_to") for role_data in roles_config.values()):
self._get_existing_principal_roles(api)
for role_name, role_data in roles_config.items():
if role_name in existing_roles_in_catalog:
logger.info(
Expand Down Expand Up @@ -1001,7 +1027,7 @@ def _create_catalog_roles(
# Update the cache with the newly created role
existing_roles_in_catalog.add(role_name)
except Exception:
logger.exception(
self._record_failure(
f"Failed to create catalog role '{role_name}' in catalog '{catalog_name}'"
)
continue
Expand All @@ -1015,6 +1041,7 @@ def _create_catalog_roles(
logger.warning(
f"Skipping assignment of catalog role '{role_name}' to non-existing principal role '{principal_role_name}'"
)
self._failure_count += 1
continue
if dry_run:
self._log_dry_run(
Expand All @@ -1040,7 +1067,7 @@ def _create_catalog_roles(
f"Assigned catalog role '{role_name}' to principal role '{principal_role_name}' successfully."
)
except Exception:
logger.exception(
self._record_failure(
f"Failed to assign catalog role '{role_name}' to principal role '{principal_role_name}'"
)
# Grant privileges
Expand Down Expand Up @@ -1115,7 +1142,7 @@ def _grant_privilege(
cmd.execute(api)
logger.info(f"Successfully granted {log_message}")
except Exception:
logger.exception(f"Failed to grant {log_message}")
self._record_failure(f"Failed to grant {log_message}")

def _create_namespaces(
self,
Expand Down Expand Up @@ -1176,7 +1203,7 @@ def _create_namespaces(
existing_namespaces.add(".".join(ns))
listed_parents.add(parent_ns)
except Exception:
logger.exception(
self._record_failure(
f"Failed to list sub-namespaces for '{parent_ns}'"
)

Expand Down Expand Up @@ -1216,7 +1243,7 @@ def _create_namespaces(
)
existing_namespaces.add(ns_name)
except Exception:
logger.exception(
self._record_failure(
f"Failed to create namespace '{ns_name}' in catalog '{catalog_name}'"
)
logger.info(
Expand Down Expand Up @@ -1254,9 +1281,16 @@ def _create_policies_and_attachments(
except NotFoundException:
policy_exists = False
except Exception:
logger.warning(
f"Could not verify existence of policy '{policy_name}', attempting creation."
)
# A real apply's create attempt determines whether this failure is terminal.
if dry_run:

@flyrain flyrain Jul 21, 2026

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.

Worth a one-line comment here on why a policy-lookup failure is counted only in dry-run. In a real apply you skip the count because the create attempt below will fail (or succeed) on its own and record its own result, but that reasoning isn't obvious from the branch alone, and the asymmetry looks like an oversight at first read. Can you add a short comment noting the create attempt covers the non-dry-run case?

self._record_failure(
f"Could not verify existence of policy '{policy_name}'"
)
else:
logger.warning(
f"Could not verify existence of policy '{policy_name}', attempting creation.",
exc_info=True,
)
policy_exists = False
if policy_exists:
logger.info(
Expand Down Expand Up @@ -1314,7 +1348,7 @@ def _create_policies_and_attachments(
)
logger.info(f"Policy '{policy_name}' created successfully.")
except Exception:
logger.exception(f"Failed to create policy '{policy_name}'")
self._record_failure(f"Failed to create policy '{policy_name}'")
continue
# Attachments
attachments = policy_data.get("attach", [])
Expand Down Expand Up @@ -1355,7 +1389,7 @@ def _create_policies_and_attachments(
cmd.execute(api)
logger.info(f"Successfully attached policy '{policy_name}'")
except Exception:
logger.exception(f"Failed to attach policy '{policy_name}'")
self._record_failure(f"Failed to attach policy '{policy_name}'")
logger.info(f"--- Finished processing policies for catalog: {catalog_name} ---")

def _validate_entity(
Expand Down
Loading
Loading