Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HP MSM Wireless Controller support #1809

Merged
merged 6 commits into from
May 22, 2019
Merged

HP MSM Wireless Controller support #1809

merged 6 commits into from
May 22, 2019

Conversation

timwsuqld
Copy link
Contributor

@timwsuqld timwsuqld commented May 16, 2019

Pre-Request Checklist

  • Passes rubocop code analysis (try rubocop --auto-correct)
  • Tests added or adapted (try rake test)
  • Changes are reflected in the documentation
  • User-visible changes appended to CHANGELOG.md

Description

Added support for the HP MSM Wireless Controller. Tested on a HP MSM720

@codecov-io
Copy link

codecov-io commented May 18, 2019

Codecov Report

Merging #1809 into master will increase coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1809      +/-   ##
==========================================
+ Coverage   63.28%   63.42%   +0.13%     
==========================================
  Files          30       30              
  Lines        1490     1490              
==========================================
+ Hits          943      945       +2     
+ Misses        547      545       -2
Impacted Files Coverage Δ
lib/oxidized/output/git.rb 19% <0%> (+1.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77cc74c...86af9ce. Read the comment docs.

@wk
Copy link
Contributor

wk commented May 18, 2019

Looks good! I've added an attribution line for the change to CHANGELOG.md, and applied the recommended Rubocop fix that prevented this PR from passing tests.

Could you add some detail as to why the show all config is being pre-processed instead of taken as-is? Is there any reason to discard the additional information? Presumably Who and When lines don't change unless the configuration itself does...

@timwsuqld
Copy link
Contributor Author

Thanks for looking at it @wk

What was kept/discarded for show all config was based on our script we wrote for rancid.

The Who/When lines are from the user that runs the show all config and so the When changes each run as it's a timestamp, and the Who changes each time as it includes the port you're connecting from.

# Who:  admin@192.168.0.2 60364 22
# When: Mon May 20 07:55:32 2019

The igmp proxy lines contain funny characters that also change between runs. I don't know why, but we excluded it in rancid, and in oxidized it also changes between each run, so I added an exclusion for it.

Did you want me to add comments to the class file as to why it's excluded?

@wk
Copy link
Contributor

wk commented May 20, 2019

Thanks for the detail! I'd suggest reworking the cleanup section for ease of comprehension along these lines (pesudocode, untested):

cmd 'show all config' do |cfg|
  cfg = cfg.each_line.reject { |line| line.match /^running configuration:/ }.join
  cfg = cfg.each_line.reject { |line| line.match /(^#\s+Who:)|(^#\s+When:)/ }.join
  cfg = cfg.each_line.reject { |line| line.match /^[ \t]*igmp proxy (upstream|downstream)/ }.join
  cfg
end

A comment around the igmp line explaining why it is removed might also benefit users, as it's generally expected that a user can recover from an archived configuration back to the original state - so if some elements are omitted breaking that promise, it's great to document them.

@timwsuqld
Copy link
Contributor Author

@wk Thanks for that. I've made updates suggested.

@wk
Copy link
Contributor

wk commented May 22, 2019

Great, thanks for the touch-ups!

@wk wk merged commit dc4da57 into ytti:master May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants