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.

Sign up for GitHub

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

Jump to bottom

refactor(pxe)!: combine dhcpd and tftpd to dnsmasq #69

Merged
merged 25 commits into from
Jul 17, 2022
Merged

refactor(pxe)!: combine dhcpd and tftpd to dnsmasq #69

merged 25 commits into from
Jul 17, 2022

Conversation

Bourne-ID
Copy link
Contributor

@Bourne-ID Bourne-ID commented Jul 10, 2022 •
edited
Loading

See #70 for proposal linked to this PR

@Bourne-ID Bourne-ID marked this pull request as draft July 10, 2022 03:53
@Bourne-ID Bourne-ID mentioned this pull request Jul 10, 2022
Closed
@Bourne-ID
Copy link
Contributor Author

Missing: Documentation. Open for implementation review.

@Bourne-ID Bourne-ID changed the title Add DNSMasq as an opt-in option combine dhcpd and tft to DNSMasq as an opt-in option Jul 11, 2022
@Bourne-ID Bourne-ID marked this pull request as ready for review July 11, 2022 03:28
@khuedoan
Copy link
Owner

Thanks! I'll test this on my metal nodes soon 😄

khuedoan
khuedoan reviewed Jul 17, 2022
View reviewed changes
Copy link
Owner

@khuedoan khuedoan left a comment

Choose a reason for hiding this comment

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

I've tested this on my metal3 node and it works flawlessly!

NAME     STATUS                     ROLES                       AGE   VERSION
metal3   Ready                      <none>                      69s   v1.23.6+k3s1

Thank you so much for this, just some comment on the default option and nitpicks:

docs/runbooks/dnsmasq.md Outdated Show resolved Hide resolved
docs/runbooks/dnsmasq.md Outdated Show resolved Hide resolved
metal/roles/pxe_server/defaults/main.yml Outdated Show resolved Hide resolved
docs/troubleshooting.md Outdated Show resolved Hide resolved
metal/roles/pxe_server/files/docker-compose.yml Outdated Show resolved Hide resolved
metal/roles/pxe_server/tasks/main.yml Outdated Show resolved Hide resolved
metal/roles/pxe_server/templates/dnsmasq.conf.j2 Outdated Show resolved Hide resolved
metal/roles/pxe_server/templates/dnsmasq.conf.j2 Outdated Show resolved Hide resolved
scripts/configure Outdated Show resolved Hide resolved
Bourne-ID and others added 12 commits July 17, 2022 13:44
@Bourne-ID @khuedoan
Correct title
2df7e62 
Co-authored-by: Khue Doan <khuedoan98@gmail.com>
@Bourne-ID @khuedoan
Remove TFTP
79fdb6c 
Co-authored-by: Khue Doan <khuedoan98@gmail.com>
@Bourne-ID @khuedoan
rename to Dnsmasq
463eee0 
Moving to use proxy by default

Co-authored-by: Khue Doan <khuedoan98@gmail.com>
@Bourne-ID @khuedoan
Rename for consistency
741380f 
Co-authored-by: Khue Doan <khuedoan98@gmail.com>
@Bourne-ID @khuedoan
Correct terminology
07b4552 
Co-authored-by: Khue Doan <khuedoan98@gmail.com>
@Bourne-ID @khuedoan
formatting
9879916 
Co-authored-by: Khue Doan <khuedoan98@gmail.com>
@Bourne-ID
Additional logging
95726b8
@Bourne-ID
proxy-by-default
af7e031
@Bourne-ID
naming consistency
1ec25e6
@Bourne-ID
naming consistency
1381dab
@Bourne-ID
Revert Dnsmasq proxy ask
5ea237b
@Bourne-ID
proxy enabled by default
c223f2a
@Bourne-ID
Copy link
Contributor Author

I believe that's all comments addressed @khuedoan - let me know if I've missed anything!

@khuedoan khuedoan changed the title combine dhcpd and tft to DNSMasq as an opt-in option refactor(pxe)!: combine dhcpd and tft to dnsmasq Jul 17, 2022
@khuedoan khuedoan changed the title refactor(pxe)!: combine dhcpd and tft to dnsmasq refactor(pxe)!: combine dhcpd and tftd to dnsmasq Jul 17, 2022
@khuedoan khuedoan changed the title refactor(pxe)!: combine dhcpd and tftd to dnsmasq refactor(pxe)!: combine dhcpd and tftpd to dnsmasq Jul 17, 2022
khuedoan
khuedoan approved these changes Jul 17, 2022
View reviewed changes
Copy link
Owner

@khuedoan khuedoan left a comment

Choose a reason for hiding this comment

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

LGTM, thank you 🎉

@khuedoan khuedoan merged commit f650c89 into khuedoan:master Jul 17, 2022
zanehala added a commit to zanehala/homelab that referenced this pull request Apr 18, 2023
@zanehala
Merge pull request 'Update all non-major dependencies' (khuedoan#69) …
419b961 
…from renovate/all-minor-patch into master

Reviewed-on: https://git.zanelab.com/zhala/homelab/pulls/69
retX0 pushed a commit to retX0/homelab that referenced this pull request Apr 7, 2024
Merge pull request 'chore(deps): update all non-major dependencies' (k…
49edf31 
…huedoan#69) from renovate/all-minor-patch into main

Reviewed-on: https://git.qos.cc/ops/homelab/pulls/69
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khuedoan khuedoan khuedoan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Bourne-ID @khuedoan @Bourne-ID-Work