deprecate/ovos_conf #138
deprecate/ovos_conf #138
Conversation
|
Happy to see the move to envvars; I have been using these everywhere for Neon, so just need to test for any unexpected side-effects |
|
@coderabbitai review |
Actions performedReview triggered.
|
WalkthroughThe Changes
Sequence Diagram(s)Sequence diagrams are omitted as the changes are primarily refactoring and simplifications, with no new features or significant modifications to control flow. Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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
Outside diff range, codebase verification and nitpick comments (2)
ovos_config/meta.py (2)
Line range hint
59-65: Setting of XDG base folder should be clearly communicated.The function
set_xdg_basenow sets the base folder name globally. Consider adding more detailed documentation or warnings to inform users about the implications of this change.
Line range hint
70-76: Global setting of configuration filename is handled properly; caution advised.The function
set_config_filenamecorrectly sets the configuration filename using environment variables. Caution should be exercised when usingset_xdg_basewithin this function to avoid unintended side effects.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- ovos_config/meta.py (5 hunks)
Additional comments not posted (7)
ovos_config/meta.py (7)
25-28: Deprecation ofsave_ovos_configis clear and well-documented.The function now logs a deprecation warning, aligning with the PR's objective to phase out the
ovos.conffile.
32-35: Deprecation ofget_ovos_default_config_pathshandled correctly.The function now logs a deprecation warning and safely returns an empty list, minimizing potential disruptions.
50-54: Retrieval of XDG base folder aligns with new configuration approach.The function
get_xdg_basecorrectly uses environment variables to determine the base folder, aligning with the PR's objectives.
81-85: Retrieval of configuration filename aligns with new configuration approach.The function
get_config_filenamecorrectly uses environment variables to determine the configuration filename, aligning with the PR's objectives.
40-43: Simplification ofis_using_xdgto always return True.This change simplifies the configuration logic. However, verify that this does not adversely affect other parts of the system that depend on this function.
Verification successful
Simplification of
is_using_xdgto always return True.This change simplifies the configuration logic. The verification confirms that this does not adversely affect other parts of the system as there are no other usages of
is_using_xdgin the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of always returning True in is_using_xdg. # Test: Search for usages of is_using_xdg and assess impact. rg --type python -A 5 $'is_using_xdg'Length of output: 67
Script:
#!/bin/bash # Description: Verify the impact of always returning True in is_using_xdg. # Test: Search for usages of is_using_xdg and assess impact. rg --glob '*.py' -A 5 'is_using_xdg'Length of output: 550
9-21: Configuration retrieval function looks good; verify directory path.The transition to using environment variables in
get_ovos_configis well-implemented. However, verify thatdirname(__file__)correctly points to the intended configuration directory.Verification successful
Configuration retrieval function looks good; directory path usage is correct.
The transition to using environment variables in
get_ovos_configis well-implemented. The use ofdirname(__file__)correctly points to the intended configuration directory.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the directory path used in get_ovos_config. # Test: Check the directory path. echo "Directory path: $(dirname $(realpath $(which python)))"Length of output: 301
Script:
#!/bin/bash # Description: Verify the directory path used in get_ovos_config. # Test: Check the directory path using available commands. script_dir=$(dirname "$0") echo "Script directory: $script_dir"Length of output: 113
Script:
#!/bin/bash # Description: Search for the usage of `dirname(__file__)` in the codebase to understand its context. rg 'dirname\(__file__\)' -A 5Length of output: 9548
Line range hint
95-103: Setting of default configuration file path is well-implemented; verify directory path.The function
set_default_configcorrectly uses an environment variable to set the default configuration file path. However, verify thatdirname(__file__)correctly points to the intended directory.Verification successful
Verified: The usage of
dirname(__file__)in theset_default_configfunction is appropriate for setting the default configuration file path. The directory path points to the location ofmeta.py, which is consistent with the intended implementation.
- The
set_default_configfunction correctly usesdirname(__file__)to set the default configuration file path relative to the directory containingmeta.py.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the directory path used in set_default_config. # Test: Check the directory path. echo "Directory path: $(dirname $(realpath $(which python)))"Length of output: 301
Script:
#!/bin/bash # Description: Verify the directory path used in set_default_config. # Test: Check the directory path using available commands. script_dir=$(cd "$(dirname "$0")" && pwd) echo "Directory path: $script_dir"Length of output: 125
Script:
#!/bin/bash # Description: Search for the set_default_config function in ovos_config/meta.py and display its implementation. ast-grep --lang python --pattern 'def set_default_config($$):' ovos_config/meta.py -A 20Length of output: 90
Script:
#!/bin/bash # Description: Search for the set_default_config function definition across all Python files in the repository. ast-grep --lang python --pattern 'def set_default_config($$):' -A 20Length of output: 70
Script:
#!/bin/bash # Description: Search for the usage of dirname(__file__) in ovos_config/meta.py. rg 'dirname\(__file__\)' ovos_config/meta.py -A 5Length of output: 594
move exclusively to env vars, ovos.conf file is of very limited use, also not a standard pattern and very uncommon in other projects, it just causes confusion to users and distro packagers (i also simply don't like it and don't want to maintain it :P )
JarbasAl
force-pushed
the
deprecate/ovos_conf
branch
from
dd2fb7c
to
40a1099
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- ovos_config/meta.py (5 hunks)
Additional comments not posted (6)
ovos_config/meta.py (6)
11-21: LGTM!The
get_ovos_configfunction effectively retrieves configuration values using environment variables with appropriate defaults.
25-28: Deprecation is well-handled.The
save_ovos_configfunction is correctly marked as deprecated with a clear warning.
32-35: Deprecation is well-handled.The
get_ovos_default_config_pathsfunction is correctly marked as deprecated with a clear warning.
39-44: Deprecation is well-handled.The
is_using_xdgfunction is correctly marked as deprecated with a clear warning and consistent behavior.
Line range hint
46-54:
LGTM!The
get_xdg_basefunction effectively retrieves the base folder name for configuration paths.
Line range hint
56-65:
LGTM!The
set_xdg_basefunction correctly sets the base folder name for configuration paths and logs the change.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #138 +/- ##
=====================================
Coverage ? 0.00%
=====================================
Files ? 10
Lines ? 788
Branches ? 0
=====================================
Hits ? 0
Misses ? 788
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/unittests/test_locations.py (2 hunks)
Additional comments not posted (1)
test/unittests/test_locations.py (1)
Line range hint
69-110:
LGTM! Ensure all environment variables are set correctly.The changes align with the PR objectives of using environment variables for configuration. The removal of the
mod_checkmock simplifies the test, and the new assertions verify the correct use of environment variables.However, verify that all necessary environment variables are set and used correctly throughout the codebase.
Verification successful
Environment Variables Set Correctly
The environment variables related to OVOS configuration are set and used appropriately in the codebase. This aligns with the intended functionality of managing configuration settings through environment variables.
test/unittests/test_locations.py: SetsOVOS_DISTRIBUTION_CONFIG,MYCROFT_SYSTEM_CONFIG,MYCROFT_WEB_CACHE, andOVOS_DEFAULT_CONFIG.ovos_config/meta.py: SetsOVOS_CONFIG_BASE_FOLDER,OVOS_CONFIG_FILENAME, andOVOS_DEFAULT_CONFIG.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all necessary environment variables are set and used correctly in the codebase. # Test: Search for usage of environment variables. Expect: All necessary environment variables are used correctly. rg --type python 'os\.environ\["OVOS_' -A 2Length of output: 76
Script:
#!/bin/bash # Description: Verify that all necessary environment variables are set and used correctly in the codebase. # Test: Search for usage of environment variables. Expect: All necessary environment variables are used correctly. rg --glob '*.py' 'os\.environ\["OVOS_' -A 2Length of output: 922
* document transformer plugins (#93) * derprecated util * derprecated util * defaults/transformer_plugins document transformer plugins and enable utterance normalizer by default * Increment Version to 0.0.13a1 * Update Changelog * Create LICENSE * update/stop_pipeline (#94) * update/stop_pipeline add stop to the pipeline by default companion to OpenVoiceOS/ovos-core#391 * Update mycroft.conf * Update mycroft.conf * Increment Version to 0.0.13a2 * Update Changelog * default utterance plugins (#99) enable https://github.com/OpenVoiceOS/ovos-utterance-corrections-plugin and https://github.com/OpenVoiceOS/ovos-utterance-plugin-cancel by default if installed * adjust adapt matcher pipeline defaults (#100) * Increment Version to 0.0.13a3 * Increment Version to 0.0.13a4 * Update Changelog * Update Changelog * Update mycroft.conf (#95) * Increment Version to 0.0.13a5 * Update Changelog * readd low adapt matches (intent pipeline) (#101) * readd low adapt matches * adjust common_qa * adjust pipeline * Increment Version to 0.0.13a6 * Update Changelog * Create dependabot.yml * fix/remove_broken_patch (#107) * fix/remove_broken_patch patch for mycroft-era `Configuration.get` (now `Configuration()`) is broken and causes .get to behave weirdly ``` self.config = dict(Configuration()) # below is False, like in mycroft.conf self.config = Configuration() # below is None ???? self.audio_enabled = self.config.get("enable_old_audioservice") ``` * bad test * signal breaking change ripping out a broken patch that we dont use anywhere, but technically a breaking change. who knows how many places are not actually reading config... bumping to 0.1.0 in case anything depends on the broken behaviour * Update version.py * Increment Version to 0.0.13a7 * Update Changelog * fix: "adapt_medium" before padatious (#110) reprioritize pipeline components * Increment Version to 0.0.13a8 * Update Changelog * improve env vars handling (#54) * Update meta.py improve env vars handling * typo * os.env priority * Update meta.py * Increment Version to 0.0.13a9 * Update Changelog * Update ovos-bus-client requirement from ~=0.0.3 to ~=0.0.8 in /requirements (#106) Updates the requirements on [ovos-bus-client](https://github.com/OpenVoiceOS/ovos-bus-client) to permit the latest version. - [Release notes](https://github.com/OpenVoiceOS/ovos-bus-client/releases) - [Changelog](https://github.com/OpenVoiceOS/ovos-bus-client/blob/dev/CHANGELOG.md) - [Commits](OpenVoiceOS/ovos-bus-client@V0.0.3...V0.0.8) --- updated-dependencies: - dependency-name: ovos-bus-client dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Increment Version to 0.0.13a10 * Update Changelog * Update ovos-backend-client requirement from <0.1.0 to <0.2.0 in /requirements (#104) Updates the requirements on [ovos-backend-client](https://github.com/OpenVoiceOS/ovos-backend-client) to permit the latest version. - [Release notes](https://github.com/OpenVoiceOS/ovos-backend-client/releases) - [Changelog](https://github.com/OpenVoiceOS/ovos-backend-client/blob/dev/CHANGELOG.md) - [Commits](OpenVoiceOS/ovos-backend-client@V0.0.1a4...V0.1.0) --- updated-dependencies: - dependency-name: ovos-backend-client dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Increment Version to 0.0.13a11 * Update Changelog * Update mycroft.conf (#112) remove unimplemented duck_while_listening, docs for non existing features are misleading change default lang detect plugin to use public servers (no extra deps) disable mpris by default in OCP, as that seems to be the cause of many issues in core 0.0.7 * Increment Version to 0.0.13a12 * Update Changelog * move to vosk (#124) * move to vosk move from pocketsphinx to vosk companion to OpenVoiceOS/ovos-dinkum-listener#113 * Update mycroft.conf * Increment Version to 0.0.13a13 * Update Changelog * feat/ocp_pipeline (#96) * feat/ocp_pipeline * feat/OCP_backends * Update mycroft.conf * Update mycroft.conf * Update mycroft.conf * Update mycroft.conf * "adapt_low", * Update mycroft.conf * Update mycroft.conf * remove deprecated flag got rid of it OpenVoiceOS/ovos-core#491 --------- Co-authored-by: JarbasAi <jarbasai@mailfence.com> Co-authored-by: JarbasAI <33701864+JarbasAl@users.noreply.github.com> * Increment Version to 0.0.13a14 * Update Changelog * fix/typo (#126) missing an X in pocketsphinX #124 * Increment Version to 0.0.13a15 * Update Changelog * Update python-dateutil requirement from ~=2.6 to ~=2.9 in /requirements (#115) Updates the requirements on [python-dateutil](https://github.com/dateutil/dateutil) to permit the latest version. - [Release notes](https://github.com/dateutil/dateutil/releases) - [Changelog](https://github.com/dateutil/dateutil/blob/master/NEWS) - [Commits](dateutil/dateutil@2.6.0...2.9.0.post0) --- updated-dependencies: - dependency-name: python-dateutil dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Increment Version to 0.0.13a16 * Update Changelog * fix: unbound local var (#127) * fix: resolve unbound local variable Closes #118 * chore: clean up issues identified by linter * revert rename * tidy up workflows * bump pytest * Increment Version to 0.0.13a17 * Update Changelog * better listener defaults (#133) use instant_listen by default, avoids OpenVoiceOS/ovos-dinkum-listener#107 until fixed enabled remove_silence by default, further making the above a better default (as listen sound gets removed by silero vad), safe to do since OpenVoiceOS/ovos-dinkum-listener#122 * Increment Version to 0.0.13a18 * Update Changelog * fix config set (#134) fix #73 fix #59 * Increment Version to 0.0.13a19 * Update Changelog * Update mycroft.conf (#136) * Update mycroft.conf add adapt/padatious default values * Update mycroft.conf * Update mycroft.conf * Update mycroft.conf * Update mycroft.conf * Increment Version to 0.0.13a20 * Update Changelog * feat: add /usr/share as a config location (#128) /usr/share/<application name> is becoming more and more common for applications to store their default configuration in. Either the application itself or distributions can use this location to ship a config that users will not touch, and instead they are expected to overwrite (parts of) the config in /etc/<application name>. This means this config file can be safely updated with new required values without having to worry about overwriting user-made change. This is useful in general but especially in case of immutable distributions who don't touch anything in /etc and /home * Increment Version to 0.0.13a21 * Update Changelog * revert to single thread (#137) use single_thread for padatious by default * Increment Version to 0.0.13a22 * Update Changelog * tweak OCP defaults (#139) default to keyword matching instead of using the new classifier (companion to OpenVoiceOS/ovos-core#502) reduce min_score from 50 to 40 so more results are considered * Increment Version to 0.0.13a23 * Update Changelog * document audio options (#141) * document audio options document some more ovos-audio options * Update mycroft.conf * Increment Version to 0.0.13a24 * Update Changelog * document "common_query" config options (#143) * Increment Version to 0.0.13a25 * Update Changelog * document "fake_barge_in" (#144) document "fake_barge_in" * Increment Version to 0.0.13a26 * Update Changelog * deprecate/ovos_conf (#138) * deprecate/ovos_conf move exclusively to env vars, ovos.conf file is of very limited use, also not a standard pattern and very uncommon in other projects, it just causes confusion to users and distro packagers (i also simply don't like it and don't want to maintain it :P ) * tests * fix test * Increment Version to 0.0.13a27 * Update Changelog * Increment Version to 0.1.0 * Update Changelog --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: JarbasAI <33701864+JarbasAl@users.noreply.github.com> Co-authored-by: JarbasAl <JarbasAl@users.noreply.github.com> Co-authored-by: Swen Gross <25036977+emphasize@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: NeonJarbas <59943014+NeonJarbas@users.noreply.github.com> Co-authored-by: JarbasAi <jarbasai@mailfence.com> Co-authored-by: Mike <mike@graywind.org> Co-authored-by: Bart Ribbers <bribbers@disroot.org>
move exclusively to env vars, ovos.conf file is of very limited use, also not a standard pattern and very uncommon in other projects, it just causes confusion to users and distro packagers
i dont think any user could make sense of the docs page for this yet https://ovos-technical-manual.openvoiceos.org/config/
(i also simply don't like it and don't want to maintain it :P )
Summary by CodeRabbit
Refactor
Bug Fixes