Skip to content
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

Jump to bottom

[configurator] Expose log level env variable #3845

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+18 −1
Open

[configurator] Expose log level env variable #3845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a10add6
[configurator] Add loglevel to config.yaml
mueslo Nov 28, 2024
02e966d
[configurator] include loglevel in run script
mueslo Nov 28, 2024
22c7ecc
Update DOCS.md
mueslo Nov 28, 2024
4380f78
Update en.yaml
mueslo Nov 28, 2024
a9698b6
set default to 'info'
mueslo Nov 28, 2024
adaf845
bump version to 5.8.1
mueslo Nov 28, 2024
d4651f8
Update CHANGELOG.md
mueslo Nov 28, 2024
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
4 changes: 4 additions & 0 deletions configurator/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## 5.8.1

- Expose configurable log level

## 5.8.0

- Update base image to Alpine 3.19
Expand Down
4 changes: 4 additions & 0 deletions configurator/DOCS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ By default, it hides the `__pycache__` folders.

A list of filenames containing SSH private keys. These can be used to allow for access to remote git repositories.

### Option: `loglevel` (optional)

The loglevel to pass to hass-configurator (default 'info').

Comment on lines +57 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance the loglevel option documentation

The documentation for the loglevel option needs more detail to help users make informed decisions.

Apply this diff to improve the documentation:

 ### Option: `loglevel` (optional)
 
-The loglevel to pass to hass-configurator (default 'info').
+Specify the log level for the File editor add-on. Valid values:
+
+- `debug`: Show all logs, including detailed debugging information
+- `info`: Show informational messages, warnings, and errors (default)
+- `warning`: Show only warning and error messages
+- `error`: Show only error messages
+- `critical`: Show only critical error messages
+
+Set to `warning` or higher to reduce log volume from HTTP requests.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
### Option: `loglevel` (optional)
The loglevel to pass to hass-configurator (default 'info').
### Option: `loglevel` (optional)
Specify the log level for the File editor add-on. Valid values:
- `debug`: Show all logs, including detailed debugging information
- `info`: Show informational messages, warnings, and errors (default)
- `warning`: Show only warning and error messages
- `error`: Show only error messages
- `critical`: Show only critical error messages
Set to `warning` or higher to reduce log volume from HTTP requests.

## Known issues and limitations

- This add-on can only be used via Ingress and has no direct access.
Expand Down
4 changes: 3 additions & 1 deletion configurator/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
version: 5.8.0
version: 5.8.1
slug: configurator
name: File editor
description: Simple browser-based file editor for Home Assistant
Expand Down Expand Up @@ -33,6 +33,7 @@ options:
- .storage
- deps
ssh_keys: []
loglevel: info
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider changing default log level to 'warning'

The PR's objective is to reduce "logspam" from HTTP request logging at 'info' level. However, keeping 'info' as the default maintains the same verbose logging behavior. Consider defaulting to 'warning' to address the stated problem while allowing users to opt-in to more verbose logging if needed.

-  loglevel: info
+  loglevel: warning
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
loglevel: info
loglevel: warning

Copy link
Author

Choose a reason for hiding this comment

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

For now the PR was only meant to allow the user to reduce logging verbosity and not change defaults. If maintainers agree I am very happy to set it to 'warning' instead, though. It is really unnecessary to log http requests by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mueslo Exposing the log level configuration without changing the default maintains current behavior, allowing users to adjust logging verbosity as needed.

panel_icon: mdi:wrench
schema:
dirsfirst: bool
Expand All @@ -42,3 +43,4 @@ schema:
- str
ssh_keys:
- str
loglevel: list(debug|info|warning|error|critical)
3 changes: 3 additions & 0 deletions configurator/rootfs/etc/s6-overlay/s6-rc.d/configurator/run
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export HC_ENFORCE_BASEPATH
export HC_GIT
export HC_HASS_API_PASSWORD
export HC_IGNORE_PATTERN
export HC_LOGLEVEL

# If any SSH key files are defined in the configuration options, add them for use by git
if bashio::config.has_value "ssh_keys"; then
Expand All @@ -31,5 +32,7 @@ HC_ENFORCE_BASEPATH=$(bashio::config 'enforce_basepath')
HC_GIT=$(bashio::config 'git')
HC_HASS_API_PASSWORD="${SUPERVISOR_TOKEN}"
HC_IGNORE_PATTERN="$(bashio::config 'ignore_pattern | join(",")')"
bashio::config.has_value 'loglevel' \
&& HC_LOGLEVEL="$(bashio::config 'loglevel')" || HC_LOGLEVEL=info
Comment on lines +35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider improvements to the log level configuration

  1. The default level of 'info' doesn't address the PR's objective of reducing logspam. Consider defaulting to 'warning' instead.
  2. There's no validation of allowed log levels, which could lead to runtime errors if invalid values are provided.
  3. The syntax could be simplified using bashio::config.get with a default value.

Consider this alternative implementation:

-bashio::config.has_value 'loglevel' \
-    && HC_LOGLEVEL="$(bashio::config 'loglevel')" || HC_LOGLEVEL=info
+# Validate log level
+LOGLEVEL="$(bashio::config.get 'loglevel' 'warning')"
+if [[ ! "${LOGLEVEL}" =~ ^(debug|info|warning|error|critical)$ ]]; then
+    bashio::log.error "Invalid log level: ${LOGLEVEL}"
+    bashio::log.error "Valid options are: debug, info, warning, error, critical"
+    bashio::exit.nok
+fi
+HC_LOGLEVEL="${LOGLEVEL}"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bashio::config.has_value 'loglevel' \
&& HC_LOGLEVEL="$(bashio::config 'loglevel')" || HC_LOGLEVEL=info
# Validate log level
LOGLEVEL="$(bashio::config.get 'loglevel' 'warning')"
if [[ ! "${LOGLEVEL}" =~ ^(debug|info|warning|error|critical)$ ]]; then
bashio::log.error "Invalid log level: ${LOGLEVEL}"
bashio::log.error "Valid options are: debug, info, warning, error, critical"
bashio::exit.nok
fi
HC_LOGLEVEL="${LOGLEVEL}"


exec hass-configurator /etc/configurator.conf
4 changes: 4 additions & 0 deletions configurator/translations/en.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,7 @@ configuration:
description: >-
A list of filenames containing SSH private keys. These can be used to
allow for access to remote git repositories.
loglevel:
name: Log level
description: >-
The log level to use (default `info`).