-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Update OpenWRT instructions in readme-vars.yml #63
Conversation
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.
Thanks for opening this pull request! Be sure to follow the pull request template!
I am a bot, here are the test results for this PR:
|
if we're not going to use uci, may as well delete the section and list wrt along with the other dnsmasq section since it's just a repeat |
Good call, I'll change it. Do you think it's a good idea to add a pointer that dnsmasq service likely needs to be restarted for the changes to take effect? I'd expect this to be true for most systems, not only OpenWRT |
yes, the restart is definitely a good call and will complement the section nicely |
I am a bot, here are the test results for this PR:
|
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.
you were going to keep the note regarding dnsmasq restart, right? I would not put /etc/init.d/dnsmasq restart, but rather something along the lines of "Ensure you restart your DNSMASQ service after the changes."
Yup, I wanted to wait for your comment and didn't see it earlier, sorry about that. Added the note now |
I am a bot, here are the test results for this PR:
|
Description:
Update the OpenWRT setup instructions, so not only the last dhcp-boot/dhcp-match pair is applied.
Benefits of this PR and context:
Resolves #62
How Has This Been Tested?
Works fine in my OpenWRT setup, booting from a VM and a "physical" device.
Source / References: