Skip to content

feat: clean up console output, add manage subcommand#23

Merged
lpillmann merged 5 commits intomainfrom
feat/output-cleanup-v2
Mar 20, 2026
Merged

feat: clean up console output, add manage subcommand#23
lpillmann merged 5 commits intomainfrom
feat/output-cleanup-v2

Conversation

@lpillmann
Copy link
Copy Markdown
Collaborator

@lpillmann lpillmann commented Mar 13, 2026

Summary

  • Remove output noise: Replace all console.log() with console.print() and remove dead Python logging framework — eliminates timestamps and file:line references from every output line
  • Rename section header: "Drop/create Snowflake objects" → "Manage Snowflake objects"
  • Add DDL summary line: Shows operation counts before the statement list (e.g. 0 CREATE, 23 ALTER (0 SET, 23 UNSET), 0 DROP) with red highlighting for DROP operations
  • Condense env loading: ~20 lines of verbose output → single Loaded N environment variables from <path> (also removes plaintext secret printing)
  • Single dry-run banner: One prominent ⚠ DRY RUN banner at the top instead of two per-section banners
  • New manage subcommand: Primary command for object management; drop_create kept as deprecated alias
  • Drive-by fixes: unclosed Rich markup tag, invalid typing.T import, privilege-skip message changed from Warning → Info

Before / After

Before:

[17:59:39] Loading environment variables  started                    utils.py:44
           PERMISSION_BOT_USER=PERMIFROST                            utils.py:62
           ...20 more lines of env vars and override listings...
[17:59:39] Drop/create Snowflake objects started                       cli.py:25
           --------------------                                     utils.py:207
           Executing in dry run mode                                utils.py:208
           --------------------                                     utils.py:209
[17:59:41] Resolving user objects                                    core.py:200
           WARNING  Skipping metadata retrieval for user admin:   inspector.py:108

After:

⚠ DRY RUN — no changes will be applied

Loaded 7 environment variables from /path/to/.env

Manage Snowflake objects started
Info: Skipping metadata retrieval for user admin: Permifrost user doesn't have DESCRIBE privileges on this object
...
DDL statements to be executed:
0 CREATE, 23 ALTER (0 SET, 23 UNSET), 0 DROP

- ALTER USER alice UNSET rsa_public_key
- ALTER USER bob SET rsa_public_key='...'
...

Manage Snowflake objects completed successfully

Guardrails

This is a clean reimplementation from main (replaces #21 which had rebase artifacts):

  • params_to_unset_if_absent for users: only ["rsa_public_key", "rsa_public_key_2"]
  • All 14 baseline tests preserved + 5 new build_summary_line tests (19 total) ✓
  • tundri/constants.py, tundri/parser.py, tundri/objects.py — no changes ✓

Test plan

  • uv run pytest tests/ --ignore=tests/integration_tests -v — 19 passed
  • uv run ruff check . && uv run ruff format --check . — clean
  • tundri manage --dry -p spec.yml works (new subcommand)
  • tundri drop_create --dry -p spec.yml still works (backwards compat)
  • tundri with no subcommand shows help instead of crashing
  • Full dry-run against sandbox — output matches After example above

🤖 Generated with Claude Code

lpillmann and others added 5 commits March 13, 2026 22:25
- Remove RichHandler, logging.basicConfig, log = getLogger, log.setLevel
  from core.py, inspector.py, and cli.py (dead code — never produced output)
- Keep import logging + urllib3 suppression lines in utils.py (needed to
  silence noisy Snowflake connector connection warnings)
- Fix broken `from typing import T` → TypeVar("T") in utils.py
- Replace all console.log() → console.print() across all 4 files
  (console.log adds [HH:MM:SS] timestamps and file:line refs — not suitable
  for user-facing CLI output)
- Update inspector warning to use [yellow]Warning:[/yellow] prefix
- Add .worktrees/ to .gitignore

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename section header 'Drop/create Snowflake objects' → 'Manage Snowflake objects'
- Consolidate dry-run banner: remove per-section log_dry_run_info() calls from
  drop_create() and permifrost(), add single call in main() before load_env_var()
- Update log_dry_run_info() to styled '⚠ DRY RUN — no changes will be applied' banner
- Condense load_env_var() output to a single summary line
  ('Loaded N environment variables from ...' or 'No .env file found, ...')
- Fix unclosed Rich markup tag in Permifrost completed message

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hting

- Add _count_operations() helper for single-pass counting of CREATE/DROP/ALTER ops
- Add _format_summary() to format counts into readable string
- Add build_summary_line() public function (returns None for empty list)
- Update print_ddl_statements() to show summary line before statements,
  with red highlighting for DROP count and DROP statements
- Add 5 tests for build_summary_line (mixed, empty, only creates, only SET,
  only UNSET)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename drop_create_objects() → manage_objects() in core.py
- Add 'manage' subcommand in cli.py via shared add_manage_args() helper
- Keep 'drop_create' as backwards-compatible alias pointing to same handler
- Move 'if len(sys.argv) == 1: parser.print_help()' to after subparsers
  are registered (ensures help is always available)
- Update argparse descriptions to use 'Manage' terminology
- Update CLAUDE.md: add 'manage' subcommand, fix module table and
  execution flow descriptions

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The message about skipping users without DESCRIBE privileges is
informational, not a warning — it's expected behaviour for admin users.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

🚀 Package published to TestPyPI!

You can test the installation with:

pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ tundri==1.3.5.dev23

View package: https://test.pypi.org/project/tundri/1.3.5.dev23/

Comment thread tundri/cli.py
parser_drop_create = subparsers.add_parser("permifrost", help="Run Permifrost")
parser_drop_create.add_argument(
parser_permifrost = subparsers.add_parser("permifrost", help="Run Permifrost")
parser_permifrost.add_argument(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Suggestion] parser_run manually duplicates the args that add_manage_args already encapsulates (-p, --dry, --users-to-skip). If the manage/drop_create args change in the future, run will silently drift out of sync.

Consider reusing the helper:

Suggested change
parser_permifrost.add_argument(
parser_run = subparsers.add_parser("run", help="Run manage and then permifrost")
add_manage_args(parser_run)
parser_run.set_defaults(func=run)

add_manage_args already sets func=manage, so you'd need to override it after the call: parser_run.set_defaults(func=run) (as shown above, set_defaults called twice merges; the second call wins).

Comment thread tundri/core.py
create += 1
elif s.startswith("DROP"):
drop += 1
elif s.startswith("ALTER"):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Suggestion] " UNSET " in s requires a space after UNSET, which holds for all current Snowflake ALTER UNSET DDL (e.g. ALTER USER foo UNSET rsa_public_key). However it would also match the substring if a SET statement's value happens to contain UNSET (e.g. ALTER USER foo SET comment = 'Please UNSET this'), silently misclassifying it.

A slightly more robust check would inspect only the SQL keyword portion:

elif s.startswith("ALTER"):
    keyword = s.split("=")[0].upper()  # strip values/assignments
    if " UNSET " in keyword or keyword.endswith(" UNSET"):
        alter_unset += 1
    else:
        alter_set += 1

Low risk in practice, but worth noting for correctness.

Comment thread tundri/core.py
summary = _format_summary(
create_count, drop_count, alter_set_count, alter_unset_count
)
if drop_count > 0:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Suggestion] Injecting Rich markup via .replace() is fragile: if drop_count happens to equal another count (e.g. create=2, drop=2), and the format string ever changes to put counts adjacent, the replacement could match unintended substrings. Since _format_summary already owns the string structure, consider passing an highlight_drops=True flag to it so the markup is built at construction time rather than patched afterwards.

@gemma-claude-assistant
Copy link
Copy Markdown

Review Summary

Overall this is a clean, well-structured PR. The rename from drop_createmanage, the console.logconsole.print migration, and the removal of the dead logging framework are all correct and improve the UX. The new build_summary_line / _count_operations / _format_summary helpers are a nice addition and are well-tested.

Three suggestions (no blockers):

  1. parser_run duplicates add_manage_args (cli.py:85) — the run subparser manually re-declares -p, --dry, and --users-to-skip instead of calling the add_manage_args helper, risking silent arg drift.

  2. UNSET detection is slightly fragile (core.py:112) — " UNSET " in s could theoretically misclassify a SET statement whose value contains the literal substring UNSET. Low probability in practice given Snowflake DDL patterns.

  3. Drop-count markup via .replace() (core.py:164) — injecting Rich markup by string-replacing the formatted summary string works now but is fragile to future format changes; better handled inside _format_summary.

Notable positives:

  • Security improvement: old load_env_var logged all env var key=value pairs (including passwords) to stdout — the new version only prints a count.
  • Unclosed Rich markup tag ([bold] without [/bold]) in the old Permifrost completion message is now fixed.
  • The from typing import T import bug (T is not a typing export) is correctly replaced with T = TypeVar("T").

@gemma-claude-assistant
Copy link
Copy Markdown

Claude review cost: $0.7149 · 13 turns · 408s · eu.anthropic.claude-sonnet-4-6

@lpillmann lpillmann requested a review from msenyonyi March 13, 2026 22:22
Copy link
Copy Markdown

@msenyonyi msenyonyi left a comment

Choose a reason for hiding this comment

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

Don't really have the context to review this, tbh - will set to approve cause it looks fine but I'd need to dedicate a bit more time to understanding Tundri

@lpillmann lpillmann merged commit 0fabc8b into main Mar 20, 2026
7 checks passed
@lpillmann lpillmann deleted the feat/output-cleanup-v2 branch March 20, 2026 17:35
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.

2 participants