-
Notifications
You must be signed in to change notification settings - Fork 52
(CISCO-71) Add support for banner/motd #586
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
Conversation
f22c281
to
5704480
Compare
@shermdog Are you done with this PR and ready for me to review? |
@mikewiebe I think it's close now. I'm going to get some beaker tests together for the puppet module and put a PR up for that so it can all be done in one fell swoop. |
@mikewiebe this should be good to merge. Added PR for puppet module. |
tests/test_banner.rb
Outdated
|
||
# TESTS | ||
|
||
def test_motd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails on my devices.
Node under test:
- name - n9k
- type - N9K-C9504
- image - bootflash:///nxos.9.2.1.bin
- version - 9.2(1)
TestBanner#test_motd = 3.62 s = F
Finished in 3.625701s, 0.2758 runs/s, 1.1032 assertions/s.
1) Failure:
TestBanner#test_motd [tests/test_banner.rb:49]:
Expected "User Access Verification\n" to be nil.
1 runs, 4 assertions, 1 failures, 0 errors, 0 skips
The behavior on my devcies is as follows.
n9k# conf t
Enter configuration commands, one per line. End with CNTL/Z.
n9k(config)# no banner motd
n9k(config)#
n9k(config)# end
n9k#
n9k# show banner motd
User Access Verification
n9k#
I see that you are setting the default to this but asserting for nil?
Should this be using refute_nil
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or.. why not check that it is set to the default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - missed updating this when I changed the behavior :)
assert_nil(Cisco::Banner.banners['default'].motd) | ||
assert_nil(banner.motd) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a multiline banner check test for versions >=
7.0(3)I7(4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any examples handy on fencing this for that release? Don't want to run up against unuspported platforms/versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the step_unless_legacy_defect()
method.
Here is an example:
skip_versions = ['7.0.3.(I2|I3)', '7.0.3.I4.[1-7]'] |
Versions that we support and need to skip would be
skip_versions = ['7.0.3.I[2-6]', '7.0.3.I7.[1-3]', '7.3', '8.[1-3]']
The fix for the n7k will likely go into 8.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry about listing a defect number for now. Just add a message indicating that multiline banners are not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we could extend
cisco-network-node-utils/tests/ciscotest.rb
Line 225 in 88e0d96
def skip_legacy_defect?(pattern, msg) |
step_unless_legacy_defect()
is to skip a specific step within a test case.
@mikewiebe fixed the broken test, extended Tested against: |
tests/test_banner.rb
Outdated
|
||
def test_multiline_motd | ||
skip_versions = ['7.0.3.I[2-6]', '7.0.3.I7.[1-3]', '7.3', '8.[1-3]'] | ||
skip_legacy_defect?(skip_versions, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the message to be something along the lines of: multiline banner configuration using nxapi not supported
@shermdog One minor comment and then should be good to merge |
This will provide support for new netdev_stdlib banner/motd type.
puppetlabs/netdev_stdlib#40