Riverbed#3317
Conversation
robertcheramy
left a comment
robertcheramy
left a comment
There was a problem hiding this comment.
Thank for your PR and also submitting a YAML-Simulation File AND a unit test. I love this, it makes reviewing the PR much easier.
Please have a look at my review and consider updating the model.
lib/oxidized/model/riverbed.rb
Outdated
| # Define comment character | ||
| comment '! ' | ||
|
|
||
| # Use procs to remove unwanted lines |
There was a problem hiding this comment.
- The "Last login" message should never appear, as Oxidized does not interpret the initial prompt.
- Do not use procs directly. Use cmd :all
There was a problem hiding this comment.
Removed the entire procs block.
lib/oxidized/model/riverbed.rb
Outdated
|
|
||
| # Remove sensitive information | ||
| cmd :secret do |cfg| | ||
| cfg.gsub! /^(\s*tacacs-server (.+ )?key) .+/, '\\1 <secret hidden>' |
There was a problem hiding this comment.
prefer space to \s when possible, as \s also matches \n and \r
There was a problem hiding this comment.
Replaced \s with explicit spaces to match only space characters.
lib/oxidized/model/riverbed.rb
Outdated
| cmd 'show hardware all' do |cfg| | ||
| # Remove the command echo and the hostname from the output | ||
| cfg = cfg.cut_head | ||
| cfg.gsub!(PROMPT, '') |
There was a problem hiding this comment.
Replaced with cfg = cfg.cut_both to remove both the first and last lines in one step.
lib/oxidized/model/riverbed.rb
Outdated
|
|
||
| # Prepend date, version, hardware, and serial information to the configuration | ||
| header = [] | ||
| header << comment("Date of version: #{Time.now.strftime('%Y-%m-%d %H:%M:%S %Z')}") |
There was a problem hiding this comment.
Printing the time is not a good idea, as oxidized will store a config after every fetch. The idea is to strip everything changing out, and store the configs in git, so you only get differences and not thousand of identical files.
There was a problem hiding this comment.
Removed the line that adds the date to the header.
lib/oxidized/model/riverbed.rb
Outdated
| # Prepend date, version, hardware, and serial information to the configuration | ||
| header = [] | ||
| header << comment("Date of version: #{Time.now.strftime('%Y-%m-%d %H:%M:%S %Z')}") | ||
| header << comment(@serial_info) if @serial_info |
There was a problem hiding this comment.
Wouldn't it be simpler to output these informations above and not store them into a variable? Most other models do so.
There was a problem hiding this comment.
Modified the code to output the extracted information directly as comments within the cmd blocks.
- Simplified the prompt definition. - Replaced `\s` with explicit spaces in regex patterns. - Used `cut_both` to simplify output trimming. - Removed dynamic date to prevent unnecessary diffs. - Outputted extracted information directly as comments.
|
Thank you for taking the time to review the PR. It's my first time doing this. I've updated the file with all the recommendations and replied to all comments. Tested and all is working as expected. |
- Fixes rubocop warnings - use "cfg = cfg.cut_both" everywhere
3 participants
Pre-Request Checklist
rubocop --auto-correct)rake test)Description
New model for Riverbed Steelheads.