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

Misc cleanup #226

Merged
merged 6 commits into from Jan 14, 2019
Merged

Misc cleanup #226

merged 6 commits into from Jan 14, 2019

Conversation

hugelgupf
Copy link
Collaborator

Based on #225. Some random stuff.

@hugelgupf hugelgupf force-pushed the misc-cleanup branch 2 times, most recently from 747c6eb to 9b4aa82 Compare January 11, 2019 01:37
@hugelgupf
Copy link
Collaborator Author

This is where rtr7/router7#19 comes in.

@codecov
Copy link

codecov bot commented Jan 11, 2019

Codecov Report

Merging #226 into master will increase coverage by 0.09%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage    72.3%   72.39%   +0.09%     
==========================================
  Files          84       83       -1     
  Lines        3676     3688      +12     
==========================================
+ Hits         2658     2670      +12     
  Misses        902      902              
  Partials      116      116
Impacted Files Coverage Δ
dhcpv4/option_class_identifier.go 100% <ø> (ø) ⬆️
dhcpv4/option_host_name.go 100% <ø> (ø) ⬆️
dhcpv4/option_broadcast_address.go 100% <ø> (ø) ⬆️
dhcpv4/option_requested_ip_address.go 100% <ø> (ø) ⬆️
dhcpv4/option_root_path.go 100% <ø> (ø) ⬆️
dhcpv4/option_domain_name.go 100% <ø> (ø) ⬆️
dhcpv4/option_vivc.go 100% <ø> (ø) ⬆️
dhcpv4/option_userclass.go 100% <ø> (ø) ⬆️
dhcpv4/option_server_identifier.go 100% <ø> (ø) ⬆️
dhcpv4/option_tftp_server_name.go 100% <ø> (ø) ⬆️
... and 26 more

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 03a987c...a955d74. Read the comment docs.

@insomniacslk
Copy link
Owner

Looks good to me, thanks! Another excellent improvement.
Once the OptVersion build failure is addressed this can be merged

Interface'd OptionCodes can print the correct human string.

It sucks because option codes are just a byte, but depending on where
you use them, they are interpreted differently. BSDP option codes !=
DHCP option codes.
Copy link
Collaborator

@pmazzini pmazzini left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Not merging yet to give @insomniacslk a chance to look at it.

@pmazzini
Copy link
Collaborator

@insomniacslk I wanted to go ahead with the merge to avoid blocking @hugelgupf but I can't since this PR breaks the router7 build, which will be fixed with rtr7/router7#19. Do I have permission to merge it even if the travis build fails?

@hugelgupf
Copy link
Collaborator Author

You're gonna have to - rtr7 doesnt use vendor or modules. And I can't submit both PRs together as one. And this dhcp one has to come first.

@insomniacslk
Copy link
Owner

as @hugelgupf said - this PR has to be merged first, then @stapelberg will have to merge rtr7/router7#19

@insomniacslk insomniacslk merged commit 84a6137 into insomniacslk:master Jan 14, 2019
@pmazzini
Copy link
Collaborator

Yes, I was willing to do that but the merge button was blocked. Thanks for merging it!

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

3 participants