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

Fixed crash when record has hardware but not ends fields #16

Merged
merged 3 commits into from
Mar 29, 2017
Merged

Fixed crash when record has hardware but not ends fields #16

merged 3 commits into from
Mar 29, 2017

Conversation

colinjc
Copy link

@colinjc colinjc commented Mar 28, 2017

When parsing our dhcpd.leases file the library was crashing on a record because it has the hardware field but not he ends field, so it tries to construct and fails with a KeyError here

        if properties['ends'] == 'never':

lease 172.23.251.150 {
starts 4 2015/09/10 00:29:00;
tstp 4 2015/09/10 00:28:44;
tsfp 4 2015/09/10 00:29:00;
atsfp 4 2015/09/10 00:29:00;
binding state free;
hardware ethernet 00:21:cc:06:94:e9;
uid "\001\000!\314\006\224\351";
}

@coveralls
Copy link

coveralls commented Mar 28, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling bff0693 on colinjc:master into 310e61b on MartijnBraam:master.

@andir
Copy link
Contributor

andir commented Mar 28, 2017

LGTM.

Out of curiousity how did you produce that lease? Static lease?

@andir
Copy link
Contributor

andir commented Mar 28, 2017

One more thing: Do you mind adding that kind of case to the tests?

@colinjc
Copy link
Author

colinjc commented Mar 28, 2017

@andir Honestly, I'm not sure how the lease was produced since I had nothing to do with the configuration of the server. I assume that you're right and it is a static lease though judging from the age.

I can quickly add a test, but I'm not sure where to add them. Should I add a record to the debian7 file and put the test in test_iscDhcpLeases.py?

@MartijnBraam
Copy link
Owner

Thanks for the contribution.

The test code should go in test_iscDhcpLeases.py and the record should go in a seperate file in the test_files directory (the same as the unittest for the options)

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 523d795 on colinjc:master into 310e61b on MartijnBraam:master.

@MartijnBraam MartijnBraam merged commit 27796a3 into MartijnBraam:master Mar 29, 2017
@MartijnBraam
Copy link
Owner

Looks great, thanks!

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

4 participants