-
Notifications
You must be signed in to change notification settings - Fork 22
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
Put dnsmasq config files in /opt/homebrew/etc #564
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.
Looks great for now - hopefully we can do some architecture sniffing in the future but let's get this running first.
bin/setup
Outdated
@@ -12,12 +12,11 @@ echo -e "You may need to enter your root password...\n" | |||
sudo mkdir -p /etc/resolver | |||
echo -e "nameserver 127.0.0.1\nport 53" | sudo tee /etc/resolver/dev.gov.uk >/dev/null | |||
|
|||
echo -e "✅ Configuring dnsmasq [/usr/local/etc/dnsmasq.conf]\n" | |||
echo "conf-dir=/usr/local/etc/dnsmasq.d,*.conf" > /usr/local/etc/dnsmasq.conf | |||
echo -e "✅ Configuring dnsmasq [/opt/homebrew/etc/dnsmasq.conf]\n" |
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.
I think it should be possible to make this cross-architecture compatible by using brew --prefix
instead of hardcoding the path:
echo -e "✅ Configuring dnsmasq [/opt/homebrew/etc/dnsmasq.conf]\n" | |
echo -e "✅ Configuring dnsmasq [$(brew --prefix)/etc/dnsmasq.conf]\n" |
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.
Ooh, interesting. I'd assumed we'd do a bit of cross-architecture sniffing when we merged into full main, but this is very low-cost, worth doing now. I'll update and confirm it works on this machine, if someone else can then try it on an intel machine.
Thanks @KludgeKML ⭐ I wonder if we can make this support both M1 and Intel, so it can be merged in earlier? E.g. in my pseudo bash
Edit: ooh @ollietreend's suggestion is better |
@ollietreend 's change made, confirmed works on M1 machine. |
Great @KludgeKML could we branch this to go into the main branch instead of the M1 one? I imagine we only want to use that branch for anything that isn't interoperable. |
@kevindew I can either change this PR to main, or I can close it and reopen on main (maybe with the 3 commits squished into one?). What's the usual preferred option? |
238c6c6
to
c6d05ee
Compare
@KludgeKML I'd change the base of this PR to main (can be done by hitting edit in the top right corner if you're not already aware). |
c6d05ee
to
1f8902c
Compare
Ug, rebase squash has interacted oddly with this PR. Going to recreate this branch from the head of main with just my changes and update the PR. |
bin/doctor Put dnsmasq config files in correct location (brew --install) rather than hardcoding to /usr/local/etc, which doesn't work on M1 machines. Add checks to bin/doctor to see if the dnsmasq additional config files exist, and if the app.dev.gov.uk host can be resolved to 127.0.0.1 using the system dig command.
1f8902c
to
3859b93
Compare
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.
This works on my Intel-based Mac 👍🏻
Why
On M1 machines, even when you can get an app running in govuk-docker, you can't currently access it via the .dev.gov.uk hostname in your browser.
What's the problem
We're using dnsmasq to resolve those hostnames into 127.0.0.1, which the nginx-docker proxy then picks up and routes to the backing app. The existing bin/setup script puts the configuration files for dnsmasq in /usr/local/etc. But on M1 machines /usr/local/etc doesn't exist, and homebrew dnsmasq looks in /opt/homebrew/etc, where the dnsmasq config files and directories already exist. This change updates those paths in bin/setup.
Any other notes
This problem wasn't caught by Doctor, which only checks:
..in this case, both of those were true, but without the other two config files nothing would work. We could perhaps update doctor to make an actual dns request (via dig maybe?) to check that the configuration is working as expected.