Skip to content

Conversation

@chrisvanheuveln
Copy link
Contributor

  • n6k has feature-set fabricpath dependency
  • Added getter/setter to feature.rb
  • test_vn_segment_vlan_based was being skipped before; added fabricpath_enable to it and now it runs on n6k

Tested both testcases on n6k, n7k, n9k

* n6k has feature-set fabricpath dependency
* Added getter/setter to feature.rb
# Get current state of the feature-set
feature_set_installed = Feature.fabricpath_installed?

config("no #{fs} ; no install #{fs}") if feature_set_installed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit Why not just call Feature.fabricpath_installed? directly here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is just a copy/paste/rename from the other feature-set tests. I think the reason I used config() here back in the day was because it's marginally faster to send both commands at once.

Copy link
Contributor

@mikewiebe mikewiebe Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not concerned about config(). I was suggesting you get rid of line 235 feature_set_installed = Feature.fabricpath_installed? and just do this.

config("no #{fs} ; no install #{fs}") if Feature.fabricpath_installed?

Copy link
Contributor

@mikewiebe mikewiebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit.

@chrisvanheuveln chrisvanheuveln merged commit 5616c63 into develop Feb 7, 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