feat: quiet-logs flag #2072
feat: quiet-logs flag #2072
Conversation
|
👋 @michrzan |
michrzan
temporarily deployed
to
elementary_test_env
GitHub Actions
Inactive
📝 WalkthroughWalkthroughAdds a new CLI flag Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
elementary/utils/log.py (1)
63-73: Root logger handler setup is consistent; consider guarding against duplicate handlersUsing
logger.propagate = False, a DEBUG logger level, and aquiet_logs-aware console handler is a solid setup that isolates the “elementary” logger tree and keeps files at full verbosity.If
set_root_logger_handlerscan be called more than once in a process (e.g., tests or re‑entry), consider clearing or reusing existing handlers to avoid accumulating multiple console/file handlers:def set_root_logger_handlers(logger_name, files_target_path, quiet_logs: bool = False): logger = logging.getLogger(logger_name) # Disable propagation to root logger to avoid duplicate logs logger.propagate = False + + # Optional: reset handlers to avoid duplicates on repeated setup + logger.handlers.clear()elementary/monitor/fetchers/alerts/alerts.py (1)
4-5: Status messages viaclick.echomeet “always visible” requirementSwitching from
logger.infotoclick.echofor “Update skipped alerts/sent alerts” ensures these final status messages are printed even when--quiet-logssuppresses INFO logs, which matches the PR goal. IfAlertsFetcheris ever reused outside CLI contexts, you may later want to inject an output callable instead of hard‑wiringclick.echo, but for current CLI usage this is fine.Also applies to: 34-35, 63-66
elementary/cli/cli.py (1)
32-38: Alignget_quiet_logsparsing with Click’s boolean semanticsCurrent parsing (
ctx.args.index("--quiet-logs")andvalue.lower() == "true") works for the documented--quiet-logs true/falseusage, but it can drift from Click’s owntype=boolbehavior (e.g., values like1,yes, or--quiet-logs=true, and env‑var defaults viaEDR_QUIET_LOGSwon’t be honored here).Consider delegating parsing to Click’s BOOL type to keep semantics in sync:
def get_quiet_logs(ctx): try: ctx_args = ctx.args idx = ctx_args.index("--quiet-logs") - return ctx_args[idx + 1].lower() == "true" + raw_value = ctx_args[idx + 1] + # Reuse Click's bool conversion so CLI arg and option parsing stay aligned. + return click.BOOL.convert(raw_value, param=None, ctx=ctx) except (ValueError, IndexError): return FalseOptionally, you could also fall back to
os.getenv("EDR_QUIET_LOGS")here if you want quiet mode configurable purely via env vars in CI.elementary/monitor/cli.py (2)
152-157:--quiet-logsoption wiring is fine; consideris_flagfor a more idiomatic UXThe new
--quiet-logsoption is correctly added to common_options and matches the documented--quiet-logs trueusage. If you ever want a more typical flag UX (--quiet-logs/--no-quiet-logswithout explicittrue/false), you could switch to anis_flagstyle declaration:- func = click.option( - "--quiet-logs", - type=bool, - default=False, - help="Minimize INFO level logs. Only WARNING and above will be shown.", - )(func) + func = click.option( + "--quiet-logs/--no-quiet-logs", + default=False, + help="Minimize INFO level logs. Only WARNING and above will be shown.", + )(func)This would also simplify
get_quiet_logssince you’d only need to detect presence of the flag, not parse string values.
333-334: Silence Ruff ARG001 by explicitly consumingquiet_logsparameters
quiet_logsis required in the signatures formonitor,report, andsend_reportso Click can pass the option, but it’s currently unused in the bodies, which triggers the RuffARG001warnings.If you don’t need per‑command behavior yet, a lightweight way to keep the parameter and quiet the linter is to explicitly consume it at the top of each function:
def monitor( ctx, @@ - maximum_columns_in_alert_samples, - quiet_logs, + maximum_columns_in_alert_samples, + quiet_logs, ): """ Get alerts on failures in dbt jobs. """ + # Currently only used by the CLI entrypoint for logger setup. + _ = quiet_logs @@ def report( @@ - target_path, - quiet_logs, + target_path, + quiet_logs, ): @@ + _ = quiet_logs @@ def send_report( @@ - target_path, - quiet_logs, + target_path, + quiet_logs, ): @@ + _ = quiet_logsThis keeps the CLI surface stable while eliminating the unused‑argument warnings until you decide to use
quiet_logsinside these commands.Also applies to: 461-462, 692-693
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
elementary/cli/cli.py(2 hunks)elementary/monitor/cli.py(4 hunks)elementary/monitor/fetchers/alerts/alerts.py(3 hunks)elementary/utils/log.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
elementary/cli/cli.py (2)
elementary/utils/log.py (1)
set_root_logger_handlers(63-73)elementary/utils/package.py (1)
get_package_version(9-10)
🪛 Ruff (0.14.8)
elementary/monitor/cli.py
333-333: Unused function argument: quiet_logs
(ARG001)
461-461: Unused function argument: quiet_logs
(ARG001)
692-692: Unused function argument: quiet_logs
(ARG001)
🔇 Additional comments (2)
elementary/utils/log.py (1)
33-41: Quiet log level selection matches intended behavior
get_console_handlercorrectly makesis_debug()take precedence overquiet_logs, and maps quiet mode toWARNINGwhile leaving the default atINFO. This aligns with “EDR_DEBUG overrides quiet mode” and “suppress INFO on console only”.elementary/cli/cli.py (1)
69-76: Logger initialization withquiet_logsis correctly orderedInitializing handlers once in
ElementaryCLI.invokeusingget_log_path(ctx)andget_quiet_logs(ctx), then gating the welcome echo and version INFO log onnot quiet_logs, cleanly implements the quiet‑mode behavior without affecting file logging.
|
Thanks @michrzan ! I know this can help many users. We'll look into it! |
arbiv
left a comment
arbiv
left a comment
There was a problem hiding this comment.
Thanks @michrzan!
My only comment is that I would change the flag to be is_flag so users only need to set it, without setting boolean value:
func = click.option(
"--quiet-logs",
is_flag=True,
default=False,
help="Minimize INFO level logs. Only WARNING and above will be shown.",
)(func)
arbiv
had a problem deploying
to
elementary_test_env
GitHub Actions
Error
michrzan
requested a deployment
to
elementary_test_env
GitHub Actions
Waiting
|
Sounds good @arbiv! Initially I noticed that other boolean parameters (e.g., I also update the Config to inlcude quiet_logs for consistency and to stop pyright from complaining about an unused variable. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
elementary/monitor/cli.py (1)
333-368: Removequiet_logsparameter from Config constructor or store it as an instance variable.The
quiet_logsparameter is accepted by the Config class (line 78 inelementary/config/config.py) but is never stored as an instance variable, making it a dead parameter. Whilequiet_logsis correctly used for logging configuration inelementary/cli/cli.pybefore Config instantiation, once passed to the Config constructor at line 367 ofelementary/monitor/cli.py, it is discarded and inaccessible to any downstream code. Either remove the parameter from the Config signature or store it asself.quiet_logsif it's needed for future use.
🧹 Nitpick comments (1)
elementary/cli/cli.py (1)
32-37: Minor: Exception handling can be simplified.The
inoperator on a list only raisesAttributeErrorifctx.argsisNoneor not iterable, notValueError. While catching both is defensive,ValueErroris unlikely in this context.🔎 Simplified exception handling
def get_quiet_logs(ctx): try: return "--quiet-logs" in ctx.args - except (ValueError, AttributeError): + except AttributeError: return False
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
elementary/cli/cli.pyelementary/config/config.pyelementary/monitor/cli.py
🧰 Additional context used
🧬 Code graph analysis (1)
elementary/cli/cli.py (2)
elementary/utils/log.py (1)
set_root_logger_handlers(63-73)elementary/utils/package.py (1)
get_package_version(9-10)
🪛 Ruff (0.14.10)
elementary/config/config.py
78-78: Unused method argument: quiet_logs
(ARG002)
🔇 Additional comments (5)
elementary/cli/cli.py (2)
69-74: LGTM! Clean conditional suppression of console output.The implementation correctly suppresses the welcome message and version info log when
quiet_logsis enabled, while preserving them for normal operation. This aligns with the PR objective to reduce console noise in CI/CD environments.
67-74: Implementation is sound; early flag detection viactx.argsworks correctly.The
get_quiet_logs()function correctly detects the--quiet-logsflag by checking its presence in Click's raw argument list. Since the flag is defined withis_flag=Trueat the subcommand level and the check occurs at theMultiCommand.invoke()level before parameter processing, the flag will reliably appear inctx.argswhenever users specify it (e.g.,edr monitor --quiet-logs). This approach is platform-independent and the try/except block properly handles edge cases.elementary/monitor/cli.py (3)
152-157: LGTM! Well-defined CLI flag with clear help text.The
--quiet-logsflag is properly defined withis_flag=True, sensible defaults, and descriptive help text. The placement incommon_optionsensures it's available across all relevant commands (monitor, report, send-report).
462-477: Consistent parameter threading in the report command.The
quiet_logsparameter is properly added to thereportcommand signature and passed to theConfigconstructor, maintaining consistency with themonitorcommand.
694-738: Consistent parameter threading in the send_report command.The
quiet_logsparameter is properly added to thesend_reportcommand signature and passed to theConfigconstructor, maintaining consistency with the other commands.
| env: str = DEFAULT_ENV, | ||
| run_dbt_deps_if_needed: Optional[bool] = None, | ||
| project_name: Optional[str] = None, | ||
| quiet_logs: Optional[bool] = None, |
There was a problem hiding this comment.
Critical: The quiet_logs parameter is not stored in the Config instance.
The parameter is accepted but never assigned to self.quiet_logs or used elsewhere in the constructor. This means the quiet_logs preference cannot be accessed by any code that uses the Config object, breaking the intended functionality.
🔎 Proposed fix to store the parameter
Add the assignment in the constructor body, similar to other parameters:
self.env = env
self.project_name = project_name
+ self.quiet_logs = quiet_logsConsider adding this around line 87, near other simple assignments.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
78-78: Unused method argument: quiet_logs
(ARG002)
🤖 Prompt for AI Agents
In elementary/config/config.py around line 78, the constructor accepts the
quiet_logs parameter but never stores it on the Config instance; add an
assignment like self.quiet_logs = quiet_logs in the constructor body (near other
simple assignments around line ~87) so the value is accessible via the Config
object and preserves the Optional[bool] type.
3 participants
Add
--quiet-logsoption to minimize console outputSummary
This PR adds a --quiet-logs flag to edr monitor, edr report, and edr send-report commands that suppresses INFO-level logs on the console, showing only WARNING and above.
Motivation
When running Elementary in CI/CD pipelines or automated environments, the verbose INFO logs can clutter output and make it harder to spot actual issues. This option allows users to run with minimal console noise while still capturing everything in the log file.
Usage
Behavior
--quiet-logs false): Shows INFO and above (unchanged behavior){target-path}/edr.logEDR_DEBUG=1): Takes precedence over--quiet-logsChanges
--quiet-logsearly and pass to logger setup--quiet-logsoption to common optionsquiet_logsparameter in handler setupclick.echo()for status messagesSummary by CodeRabbit
New Features
User-facing changes
✏️ Tip: You can customize this high-level summary in your review settings.