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

Add helper to deconfigure an interface #107

Merged
merged 3 commits into from
Aug 20, 2019
Merged

Add helper to deconfigure an interface #107

merged 3 commits into from
Aug 20, 2019

Conversation

ConnorRigby
Copy link
Member

I find myself writing this helper for every project that uses VintageNet, so i thought i'd just put it here.

@fhunleth
Copy link
Member

I like this. The only hesitation is that if you deconfigure and call configured_interfaces/0, you'll still get the interface. What do you think of adding a filter on configured_interfaces/0 to prune it of everything with a Null configuration?

In case you're wondering why I never implemented a complete delete, the reason is that I ran into a race condition with a back-to-back delete and re-add at one point. I thought that the benefit of removing a configuration was so small that it wasn't worth maintaining.

@ConnorRigby
Copy link
Member Author

What do you think of adding a filter on configured_interfaces/0 to prune it of everything with a Null configuration?

This seems straightforward enough. Update incoming

@fhunleth
Copy link
Member

Oh no, coverage has dropped below 80% and that changes the badge color. Could you add a short unit test?

@ConnorRigby
Copy link
Member Author

@fhunleth i fixed the test i broke. Should i add another? Coverage seems to be right on the cusp of 80%

@fhunleth
Copy link
Member

Yeah, it would be good to have at least one call to deconfigure in the unit tests.

@fhunleth fhunleth merged commit b22f567 into master Aug 20, 2019
@fhunleth fhunleth deleted the deconfigure-helper branch August 20, 2019 16:43
@fhunleth
Copy link
Member

Great! And thanks for adding the test.

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.

None yet

2 participants