Allow using regexp for model_map and group_map keys #3362
Conversation
Use case is that the source returns string which has unrelated and variant data but the actual model is included somewhere in this data For example source could have space separated list of tags and one of these tags could be model name. Eg 'procurve switch'. Closes #3360
We want to retain regexp keys for #3360
|
I honestly cannot remember why I am normalising all keys to strings, but tests at least pass even if we don't normalise them. |
|
Another option, which doesn't require changing the key_to_s would be to introduce new config stanza like. source:
map:
model:
"oxidized_model": "source_model"By swapping the order, we would guarantee the key is string, and value could be potentially any other class. How we could transition to this new config stanza is that we could check existence of old, if old is found, we could emit warning that it's being deprecated and removed later in X, potentially link to documentation. Then we swap key/value and process the old and new stanza the same way. I don't feel strongly. |
|
Closing and reopening, as I want to see what CodeRabbit does. Apparently nothing, guess it excludes reopened ones and wants to see fresh. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe pull request introduces enhancements to Oxidized's configuration system, focusing on improving device model and group mapping flexibility. The changes allow the Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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 (
|
![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 (4)
lib/oxidized/source/source.rb (1)
11-17: Consider performance optimization for map_value.The current implementation iterates through all keys for every lookup. For large maps with regex keys, this could be inefficient. Consider:
- Separating regex and non-regex keys
- Checking exact matches first
- Caching compiled regexes
def map_value(map_hash, original_value) + # First try exact matches (faster) + return map_hash[original_value] if map_hash.key?(original_value) + + # Then try regex matches map_hash.each do |key, new_value| mthd = key.instance_of?(Regexp) ? :match : :eql? return new_value if original_value.send(mthd, key) end original_value endspec/source/source_spec.rb (1)
4-31: Enhance test coverage.While the current tests cover basic functionality, consider adding:
- Edge cases (nil, empty string)
- Performance tests for large maps
- Tests for the map_group method
- Tests for overlapping regex patterns
describe Oxidized::Source do describe '#map_model' do before(:each) do Asetus.any_instance.expects(:load) Asetus.any_instance.expects(:create).returns(false) # Set :home_dir to make sure the OXIDIZED_HOME environment variable is not used Oxidized::Config.load({ home_dir: '/cfg_path/' }) yaml = %( juniper: junos !ruby/regexp /procurve/: procurve + !ruby/regexp /^$/: empty_model + !ruby/regexp /cisco|ios/: ios ) Oxidized.config.model_map = YAML.unsafe_load(yaml) @source = Oxidized::Source::Source.new end + it 'handles nil input' do + _(@source.map_model(nil)).must_equal nil + end + + it 'handles empty string input' do + _(@source.map_model('')).must_equal 'empty_model' + end + + it 'handles overlapping regex patterns' do + _(@source.map_model('cisco-ios')).must_equal 'ios' + end end + + describe '#map_group' do + # Add similar tests for group mapping + end enddocs/Configuration.md (1)
229-229: Enhance regex documentation.While the example shows how to use regex, consider adding:
- Explanation of regex pattern syntax
- Best practices for pattern matching
- Performance implications of using regex
- Examples of common patterns
model_map: cisco: ios juniper: junos !ruby/regexp /procurve/: procurve + # Regex patterns are matched against the entire string + # Examples: + # Match exact word: !ruby/regexp /^procurve$/ + # Match at start: !ruby/regexp /^cisco/ + # Match anywhere: !ruby/regexp /juniper/ + # Case insensitive: !ruby/regexp /(?i)cisco/ + # Note: Complex patterns may impact performanceCHANGELOG.md (1)
11-11: LGTM! Consider enhancing the regexp entry description.The changelog entries are well-formatted and follow the Keep a Changelog convention. However, the regexp entry could be more descriptive about the impact and benefits of this change.
Consider expanding the regexp entry to:
-config: allow model_map and group_map keys to be regexp. Fixes #3360 (@ytti) +config: allow model_map and group_map keys to be regexp for flexible device matching. Fixes #3360 (@ytti)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)docs/Configuration.md(1 hunks)lib/oxidized/config.rb(1 hunks)lib/oxidized/source/source.rb(1 hunks)spec/source/source_spec.rb(1 hunks)
🔇 Additional comments (1)
lib/oxidized/config.rb (1)
22-22: Verify the impact of removing key_to_s.The removal of
key_to_s: trueallows regex keys in maps but might affect other configuration values. Let's verify there are no unintended consequences.✅ Verification successful
Removal of key_to_s is safe
The codebase consistently converts keys to strings before configuration lookups, making it resilient to non-string keys in configuration maps. The explicit key conversion in the codebase ensures compatibility with both string and non-string keys.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct key access or string operations on configuration values rg -A 2 "config\.[a-zA-Z_]+\[.+\]|\.to_s|\.is_a\?\s*\(?String\)?"Length of output: 16672
Docstrings generation was requested by @ytti. * #3362 (comment) The following files were modified: * `lib/oxidized/source/source.rb`
|
Note We have generated docstrings for this pull request, at #3366 |
|
I'd like to extend the documentation:
|
|
I'm ok with that. Personally I don't write or read ever comments which is English version of the code, to me it is non-sensical to maintain two versions of code, one in formal language another in natural language. I don't read them, because I don't trust that they are in sync, or that the English version ever was accurate about formal version. I also try to write methods which are short, do one thing without side effects and fit in mental space, in which case it is often faster to just read the code. I think comments make more sense when the 'why' is important, not the 'what', like when you know what the code does, but you might not understand why it is needed. Or if code is particularly tricky and I couldn't figure out how to write it elegantly and simply. But each to their own, and I'm not going to reject any style of commenting. |
|
I like using comments because they help me understand the code faster later on, and I hope it helps others too. The PR is good for me an can be merged. |
Use case is that the source returns string which has unrelated and variant data but the actual model is included somewhere in this data
For example source could have space separated list of tags and one of these tags could be model name. Eg 'procurve switch'.
Closes #3360
Summary by CodeRabbit
New Features
model_mapandgroup_mapkeys.Documentation
Improvements
map_valuemethod to support flexible key matching.Tests