From 2218de03999c946ae79be6577337a7106bda09c2 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Fri, 24 May 2024 13:55:37 +0000 Subject: [PATCH] management: Disable Sentry for management commands run interactively. This adds `--automated` and `--no-automated` flags to all Zulip management commands, whose default is based on if STDIN is a TTY. This enables cron jobs and supervisor commands to continue to report to Sentry, and manually-run commands (when reporting to Sentry does not provide value, since the user can see them) to not. Note that this only applies to Zulip commands -- core Django commands (e.g. `./manage.py`) do not grow support for `--automated` and will always report exceptions to Sentry. `manage.py` subcommands in the `upgrade` and `restart-server` paths are marked as `--automated`, since those may be run semi-unattended, and they are useful to log to Sentry. --- docs/subsystems/management-commands.md | 16 +++++++++------- .../send_zulip_update_announcements.hook | 2 +- scripts/lib/upgrade-zulip-stage-3 | 5 +++-- scripts/restart-server | 2 +- zerver/lib/management.py | 17 ++++++++++++++++- 5 files changed, 30 insertions(+), 12 deletions(-) diff --git a/docs/subsystems/management-commands.md b/docs/subsystems/management-commands.md index 0327890c26..9efa91ccbf 100644 --- a/docs/subsystems/management-commands.md +++ b/docs/subsystems/management-commands.md @@ -38,13 +38,15 @@ command to write a new one. Some good examples are is good, but we have a few pieces advice specific to the Zulip project. -- If you need to access a realm or user, use the `ZulipBaseCommand` - class in `zerver/lib/management.py` so you don't need to write the - tedious code of looking those objects up. This is especially - important for users, since the library handles the issues around - looking up users by email well (if there's a unique user with that - email, just modify it without requiring the user to specify the - realm as well, but if there's a collision, throw a nice error). +- Inherit from the `ZulipBaseCommand` class in + `zerver/lib/management.py`; this will add some helpful general + flags, as well as tools for adding and parsing `--realm` and + `--user` flags, so you don't need to write the tedious code of + looking those objects up. This is especially important for users, + since the library handles the issues around looking up users by + email well (if there's a unique user with that email, just modify it + without requiring the user to specify the realm as well, but if + there's a collision, throw a nice error). - Avoid writing a lot of code in management commands; management commands are annoying to unit test, and thus easier to maintain if all the interesting logic is in a nice function that is unit tested diff --git a/puppet/zulip/files/hooks/post-deploy.d/send_zulip_update_announcements.hook b/puppet/zulip/files/hooks/post-deploy.d/send_zulip_update_announcements.hook index 8c26f3e18d..88f6afc854 100644 --- a/puppet/zulip/files/hooks/post-deploy.d/send_zulip_update_announcements.hook +++ b/puppet/zulip/files/hooks/post-deploy.d/send_zulip_update_announcements.hook @@ -2,4 +2,4 @@ set -e -/home/zulip/deployments/current/manage.py send_zulip_update_announcements +/home/zulip/deployments/current/manage.py send_zulip_update_announcements --automated diff --git a/scripts/lib/upgrade-zulip-stage-3 b/scripts/lib/upgrade-zulip-stage-3 index 31028efa04..ecfa13a212 100755 --- a/scripts/lib/upgrade-zulip-stage-3 +++ b/scripts/lib/upgrade-zulip-stage-3 @@ -158,7 +158,8 @@ def fill_memcached_caches() -> None: if HAS_FILLED_CACHES or migrations_needed: return subprocess.check_call( - ["./manage.py", "fill_memcached_caches", "--skip-checks"], preexec_fn=su_to_zulip + ["./manage.py", "fill_memcached_caches", "--automated", "--skip-checks"], + preexec_fn=su_to_zulip, ) HAS_FILLED_CACHES = True @@ -486,7 +487,7 @@ if args.audit_fts_indexes: logging.info("Correcting full-text search indexes for updated dictionary files") logging.info("This may take a while but the server should work while it runs.") subprocess.check_call( - ["./manage.py", "audit_fts_indexes", "--skip-checks"], preexec_fn=su_to_zulip + ["./manage.py", "audit_fts_indexes", "--automated", "--skip-checks"], preexec_fn=su_to_zulip ) if not args.skip_purge_old_deployments: diff --git a/scripts/restart-server b/scripts/restart-server index 46f9eb1854..e910604185 100755 --- a/scripts/restart-server +++ b/scripts/restart-server @@ -56,7 +56,7 @@ if not args.skip_checks: if args.fill_cache: logging.info("Filling memcached caches") - subprocess.check_call(["./manage.py", "fill_memcached_caches", "--skip-checks"]) + subprocess.check_call(["./manage.py", "fill_memcached_caches", "--automated", "--skip-checks"]) current_symlink = os.path.join(DEPLOYMENTS_DIR, "current") last_symlink = os.path.join(DEPLOYMENTS_DIR, "last") diff --git a/zerver/lib/management.py b/zerver/lib/management.py index 4463d1472b..a818989b52 100644 --- a/zerver/lib/management.py +++ b/zerver/lib/management.py @@ -2,7 +2,7 @@ import logging import os import sys -from argparse import ArgumentParser, RawTextHelpFormatter, _ActionsContainer +from argparse import ArgumentParser, BooleanOptionalAction, RawTextHelpFormatter, _ActionsContainer from dataclasses import dataclass from functools import reduce, wraps from typing import Any, Dict, Optional, Protocol @@ -78,9 +78,24 @@ class ZulipBaseCommand(BaseCommand): @override def create_parser(self, prog_name: str, subcommand: str, **kwargs: Any) -> CommandParser: parser = super().create_parser(prog_name, subcommand, **kwargs) + parser.add_argument( + "--automated", + help="This command is run non-interactively (enables Sentry, etc)", + action=BooleanOptionalAction, + default=not sys.stdin.isatty(), + ) parser.formatter_class = RawTextHelpFormatter return parser + @override + def execute(self, *args: Any, **options: Any) -> None: + if settings.SENTRY_DSN and not options["automated"]: # nocoverage + import sentry_sdk + + # This deactivates Sentry + sentry_sdk.init() + super().execute(*args, **options) + def add_realm_args( self, parser: ArgumentParser, *, required: bool = False, help: Optional[str] = None ) -> None: