Support for split WEBHOOK_SERVER_DATA_DIR and WEBHOOK_SERVER_CONFIG_DIR#731
Support for split WEBHOOK_SERVER_DATA_DIR and WEBHOOK_SERVER_CONFIG_DIR#731
WEBHOOK_SERVER_DATA_DIR and WEBHOOK_SERVER_CONFIG_DIR#731Conversation
If user wants to have the config dir seperated from the data dir he can pass the above os env to set it. If WEBHOOK_SERVER_CONFIG_DIR not passed, WEBHOOK_SERVER_DATA_DIR is used.
WalkthroughThis change introduces support for specifying a separate configuration directory for the GitHub webhook server using the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
PR will be approved when the following conditions are met:
Approvers and Reviewers
Supported /retest check runs
Supported labels
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
83-86: Line length exceeds recommended limitThe description for
WEBHOOK_SERVER_CONFIG_DIRexceeds the recommended line length of 120 characters. Consider reformatting for better readability.- - `WEBHOOK_SERVER_CONFIG_DIR`: Path to the config directory where the `config.yaml` and `*-private-key.pem` (Github APP cert) file is located. + - `WEBHOOK_SERVER_CONFIG_DIR`: Path to the config directory where the `config.yaml` and + `*-private-key.pem` (Github APP cert) files are located.Also, I recommend changing "file is located" to "files are located" since there are multiple files mentioned.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~83-~83: Loose punctuation mark.
Context: ...here the . -WEBHOOK_SERVER_CONFIG_DIR: Path to the config directory where the ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
83-83: Line length
Expected: 120; Actual: 142(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(3 hunks)docker-compose-example.yaml(1 hunks)webhook_server_container/libs/config.py(1 hunks)webhook_server_container/libs/github_api.py(1 hunks)webhook_server_container/utils/github_repository_settings.py(1 hunks)webhook_server_container/utils/helpers.py(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...o be stored. - WEBHOOK_SERVER_DATA_DIR: Path to the data directory where the . ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~83-~83: Loose punctuation mark.
Context: ...here the . - WEBHOOK_SERVER_CONFIG_DIR: Path to the config directory where the ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
README.md
83-83: Line length
Expected: 120; Actual: 142
(MD013, line-length)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tox
- GitHub Check: python-module-install
- GitHub Check: pre-commit
- GitHub Check: build-container
🔇 Additional comments (6)
webhook_server_container/utils/github_repository_settings.py (1)
393-393: Usingconfig_dirfor the private key location aligns with the PR objectives.This change correctly uses the new
config_dirproperty to locate the private key file, which is appropriately considered configuration rather than runtime data. This change supports the goal of separating configuration from data storage.webhook_server_container/libs/github_api.py (1)
262-266: Good enhancement to error handling for file operations.The added try-except block improves robustness by gracefully handling file write errors instead of allowing exceptions to propagate. This ensures the application continues functioning even if it can't write to the color file.
webhook_server_container/utils/helpers.py (1)
4-4: Correctly updated log file path resolution.Adding the
osimport and constructing proper file paths for logs ensures they are correctly stored in the data directory. This change properly maintains the separation between configuration and data files.Also applies to: 25-26
README.md (1)
45-45: Repository URL update is consistentThe change from the old repository URL to
ghcr.io/myk-org/github-webhook-server:latestis consistently applied in both locations.Also applies to: 196-196
webhook_server_container/libs/config.py (2)
13-14: Proper implementation of config directory separationGood implementation of the new
WEBHOOK_SERVER_CONFIG_DIRenvironment variable that defaults to the data directory when not set. This matches the behavior described in the documentation and provides a clean way to separate configuration files from data files.
1-72:Details
✅ Verification successful
Verify other code dependencies
Make sure other parts of the codebase that might rely on the location of the config file have been updated to use
config_dirinstead ofdata_dirwhere appropriate.
🏁 Script executed:
#!/bin/bash # Find any references to config.yaml that might need updating echo "Finding references to config file path:" rg "config\.yaml|config_path" --type py # Check if there are any references to private key files that need to be updated echo "Finding references to private key files:" rg "private-key\.pem" --type pyLength of output: 1759
No outstanding config path dependencies
All references toconfig.yamlandwebhook-server.private-key.pemalready useconfig_dir. No other parts of the codebase rely on the olddata_dirfor config paths.
webhook_server_container/utils/github_repository_settings.pycorrectly usesconfig_.config_dirfor the private key.- Tests patch
Config.config_pathand aren’t affected.
| - WEBHOOK_SERVER_CONFIG_DIR: /path/to/config/dir # Default to WEBHOOK_SERVER_DATA_DIR if not set | ||
| - WEBHOOK_SERVER_DATA_DIR: /path/to/data/dir |
There was a problem hiding this comment.
Fix environment variable syntax in Docker Compose file.
Docker Compose environment variables should use = for assignment, not :.
Apply this correction:
- - WEBHOOK_SERVER_CONFIG_DIR: /path/to/config/dir # Default to WEBHOOK_SERVER_DATA_DIR if not set
- - WEBHOOK_SERVER_DATA_DIR: /path/to/data/dir
+ - WEBHOOK_SERVER_CONFIG_DIR=/path/to/config/dir # Default to WEBHOOK_SERVER_DATA_DIR if not set
+ - WEBHOOK_SERVER_DATA_DIR=/path/to/data/dir📝 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.
| - WEBHOOK_SERVER_CONFIG_DIR: /path/to/config/dir # Default to WEBHOOK_SERVER_DATA_DIR if not set | |
| - WEBHOOK_SERVER_DATA_DIR: /path/to/data/dir | |
| - WEBHOOK_SERVER_CONFIG_DIR=/path/to/config/dir # Default to WEBHOOK_SERVER_DATA_DIR if not set | |
| - WEBHOOK_SERVER_DATA_DIR=/path/to/data/dir |
|
|
||
| - `WEBHOOK_SERVER_LOG_FILE`: Path to the log file where the server logs are to be stored. | ||
| - `WEBHOOK_SERVER_DATA_DIR`: Path to the data directory where the `config.yaml` file is located. | ||
| - `WEBHOOK_SERVER_DATA_DIR`: Path to the data directory where the . |
There was a problem hiding this comment.
Fix incomplete description of WEBHOOK_SERVER_DATA_DIR
The description for WEBHOOK_SERVER_DATA_DIR ends abruptly with "where the ." - it should specify what the data directory contains or its purpose.
- - `WEBHOOK_SERVER_DATA_DIR`: Path to the data directory where the .
+ - `WEBHOOK_SERVER_DATA_DIR`: Path to the data directory where the server stores its data.📝 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.
| - `WEBHOOK_SERVER_DATA_DIR`: Path to the data directory where the . | |
| - `WEBHOOK_SERVER_DATA_DIR`: Path to the data directory where the server stores its data. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...o be stored. - WEBHOOK_SERVER_DATA_DIR: Path to the data directory where the . ...
(UNLIKELY_OPENING_PUNCTUATION)
2 participants
If user wants to have the config dir seperated from the data dir he can pass the above os env to set it.
If WEBHOOK_SERVER_CONFIG_DIR not passed, WEBHOOK_SERVER_DATA_DIR is used.
Summary by CodeRabbit