feat: Allow to include files for RR root configuration #38
Conversation
WalkthroughThe changes introduced in this diff primarily focus on enhancing the configuration management of the application. A new function Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (8)
- configuration.go (1 hunks)
- plugin.go (4 hunks)
- tests/configs/.rr.include1-sub1.yaml (1 hunks)
- tests/configs/.rr.include1-sub2.yaml (1 hunks)
- tests/configs/.rr.include1.yaml (1 hunks)
- tests/configs/.rr.include2-sub1.yaml (1 hunks)
- tests/configs/.rr.include2.yaml (1 hunks)
- tests/plugin_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/configs/.rr.include2-sub1.yaml
Additional comments: 8
tests/configs/.rr.include1-sub2.yaml (1)
- 1-6: The new configuration file looks good. It's clear and concise, and it follows the YAML syntax correctly. The logging settings are well defined. Ensure that these settings are appropriate for your application's needs.
tests/configs/.rr.include1-sub1.yaml (1)
- 1-16: This new configuration file looks good. It specifies the server command, relay method, HTTP address, middleware, pool settings, and a reload interval. Ensure that these settings are appropriate for the intended use case.
tests/configs/.rr.include1.yaml (1)
- 1-9: The configuration file seems to be correctly formatted and includes the necessary fields. Ensure that the included files ".rr.include1-sub1.yaml" and ".rr.include1-sub2.yaml" exist and are in the correct location. Also, verify that the reload interval and patterns are appropriate for your use case.
tests/configs/.rr.include2.yaml (1)
- 1-8: The configuration file seems to be correctly formatted and includes another configuration file,
.rr.include2-sub1.yaml. Ensure that this included file exists and is in the correct location. The reload interval is set to 1 second and the pattern is set to watch for changes in.phpfiles. This seems reasonable if the server is expected to handle PHP files and needs to reload frequently.configuration.go (1)
- 10-29: The function
getConfigurationreads configuration settings from a file and environment variables using theviperpackage. It also retrieves a configuration version from the settings. Error handling is done for configuration reading and version retrieval. Ensure that the function is used correctly throughout the codebase and that the configuration files and environment variables are set up properly.plugin.go (3)
4-10: The new import "path/filepath" is used for handling file paths in a cross-platform way. This is a good practice.
15-21: The new constant "includeKey" is used to get the "include" key from the configuration. This is a good practice to avoid hardcoding strings.
148-153: The comment explains why the version check is only required for tests. This is a good practice to explain the context and reasoning behind the code.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- configuration.go (1 hunks)
- plugin.go (5 hunks)
- tests/plugin_test.go (2 hunks)
Additional comments: 6
configuration.go (1)
- 10-28: The
getConfigurationfunction is well-structured and follows good practices. It uses theviperpackage to read configuration settings from a file and environment variables, and it handles errors appropriately. However, as previously mentioned, consider adding additional checks for the version format or range.ver := viper.Get(versionKey) if ver == nil { return nil, "", errors.Str("rr configuration file should contain a version e.g: version: 2.7") } // Add additional checks for version format or range heretests/plugin_test.go (1)
- 73-83: The test
TestErrorWhenIncludedConfigHaveDifferentVersionThenRootchecks if an error is thrown when an included config has a different version than the root. This is a good practice to ensure version consistency across included files.plugin.go (4)
4-10: The new import statement for "path/filepath" is added correctly.
15-21: The new constant "includeKey" is added correctly.
70-86: The function
handleIncludedConfigurationFilesis called correctly in theInitfunction. Ensure that all error handling is done correctly.124-129: The condition for setting the default version is correct.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- plugin.go (5 hunks)
Additional comments: 5
plugin.go (5)
4-10: The
path/filepathpackage is added to handle file paths in a cross-platform manner. This is a good practice for ensuring the code works across different operating systems.15-21: The
includeKeyconstant is added to handle included configuration files. This is a good practice for code readability and maintainability.70-86: The
handleIncludedConfigurationFilesfunction is called within theInitfunction. Ensure that the function is properly handling errors and that the error messages are informative enough for debugging.198-229: The
handleIncludedConfigurationFilesfunction is added to handle included configuration files. It retrieves the configuration from each included file, validates the version, and merges the configurations. This function should be tested thoroughly to ensure it correctly handles different scenarios, such as missing files, incorrect file formats, version mismatches, and merging conflicts.124-129: The condition checks for the RoadRunner version and sets it to the default version if it's not provided or if it's "local". This is a good practice for ensuring backward compatibility and handling edge cases.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/plugin_test.go (2 hunks)
Additional comments: 1
tests/plugin_test.go (1)
- 73-83: The test
TestErrorWhenIncludedConfigHaveDifferentVersionThenRootchecks if an error is thrown when an included configuration file has a different version than the root file. This is a good practice to ensure version consistency across included files.
Signed-off-by: Valery Piashchynski <[email protected]>
|
I'll add a few updates on your PR if you don't mind. Just some hidden logic:
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- include.go (1 hunks)
- plugin.go (5 hunks)
- tests/plugin_test.go (3 hunks)
Files not summarized due to errors (3)
- plugin.go: Error: Server error. Please try again later.
- tests/plugin_test.go: Error: Server error. Please try again later.
- include.go: Error: Server error. Please try again later.
Files not reviewed due to errors (2)
- include.go (Error: Server error. Please try again later.)
- plugin.go (Error: Server error. Please try again later.)
Additional comments: 2
tests/plugin_test.go (2)
613-635: The test
TestIncludingConfigschecks if the configuration values are correctly retrieved from the root and included files. It also verifies if the values are overridden correctly. This is a good practice as it ensures the new feature works as expected.637-648: The test
TestErrorWhenIncludedConfigHaveDifferentVersionThenRootchecks if an error is thrown when the version in the included file is different from the root. This is a good practice as it ensures the system behaves as expected when encountering inconsistencies.
Signed-off-by: Valery Piashchynski <[email protected]>
|
Thanks @cv65kr 👍 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- expand.go (2 hunks)
- include.go (1 hunks)
- plugin.go (5 hunks)
Files not reviewed due to errors (2)
- include.go (Error: Server error. Please try again later.)
- plugin.go (Error: Server error. Please try again later.)
Additional comments: 1
expand.go (1)
- 94-94: The function
parseEnvDefaultis not defined in this file. Ensure that it is imported or defined elsewhere in the codebase.
| func expandEnvViper(v *viper.Viper) { | ||
| for _, key := range v.AllKeys() { | ||
| val := v.Get(key) | ||
| switch t := val.(type) { | ||
| case string: | ||
| // for string expand it | ||
| v.Set(key, parseEnvDefault(t)) | ||
| case []any: | ||
| // for slice -> check if it's a slice of strings | ||
| strArr := make([]string, 0, len(t)) | ||
| for i := 0; i < len(t); i++ { | ||
| if valStr, ok := t[i].(string); ok { | ||
| strArr = append(strArr, parseEnvDefault(valStr)) | ||
| continue | ||
| } | ||
|
|
||
| v.Set(key, val) | ||
| } | ||
|
|
||
| // we should set the whole array | ||
| if len(strArr) > 0 { | ||
| v.Set(key, strArr) | ||
| } | ||
| default: | ||
| v.Set(key, val) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The expandEnvViper function iterates over all keys in the viper.Viper instance and expands environment variables in string values. It also handles expansion for slices of strings. However, it doesn't handle nested structures like maps or slices of maps. If your configuration can contain such structures, you might want to enhance this function to handle them.
| // for slice -> check if it's a slice of strings | ||
| strArr := make([]string, 0, len(t)) | ||
| for i := 0; i < len(t); i++ { | ||
| if valStr, ok := t[i].(string); ok { | ||
| strArr = append(strArr, parseEnvDefault(valStr)) | ||
| continue | ||
| } | ||
|
|
||
| v.Set(key, val) | ||
| } | ||
|
|
||
| // we should set the whole array | ||
| if len(strArr) > 0 { | ||
| v.Set(key, strArr) | ||
| } |
There was a problem hiding this comment.
This block handles slices, but it assumes that all elements in the slice are of the same type. If a slice contains mixed types (e.g., strings and integers), this could lead to unexpected behavior. Consider adding type checking for each element in the slice.
| v.Set(key, strArr) | ||
| } | ||
| default: | ||
| v.Set(key, val) |
There was a problem hiding this comment.
This line sets the value of the key to its original value, which is unnecessary as it doesn't change anything. Consider removing this line.
- v.Set(key, val)Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| v.Set(key, val) |
Reason for This PR
closes: roadrunner-server/roadrunner#935
Description of Changes
Merging configurations from multiple files by
includepropertyLicense Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit