Conversation
| } | ||
|
|
||
| // Is libnss-resolver in use | ||
| if _, err := os.Stat("/etc/resolver"); err == nil { |
There was a problem hiding this comment.
To be clear, we will set up *.vm support via both these mechanisms if both happen to be present? I believe we discussed that being safe and the approach, but wanted to confirm.
| * Start Docker in Docker | ||
| * `docker run --privileged -it --name dind -d docker:dind` | ||
| * Start the container for the distro you want, mounting a linux targeted `rig` into it and linking it to the Docker in Docker image. | ||
| * `docker run -it -v $PWD/build/linux/rig:/usr/bin/rig --link dind:docker test-centos bash` |
grayside
left a comment
grayside
left a comment
There was a problem hiding this comment.
First pass with minor points. I think I'll need to review this again.
commands/dns.go
Outdated
|
|
||
| // ConfigureMac configures DNS resolution and network routing | ||
| func (cmd *DNS) configureMacRoutes(machine Machine) { | ||
| cmd.out.Verbose.Print("Configuring network routing for macOS") |
There was a problem hiding this comment.
Is this log valuable, as it directly follows this info log
Setting up local networking (may require your admin password)
There was a problem hiding this comment.
Maybe not I can remove those.
| cmd.removeHostFilter(machineIP) | ||
| } | ||
| exec.Command("sudo", "route", "-n", "delete", "-net", "172.17.0.0").Run() | ||
| util.StreamCommand(exec.Command("sudo", "route", "-n", "add", "172.17.0.0/16", machineIP)) |
There was a problem hiding this comment.
Why is delete Run() and add StreamCommand()?
There was a problem hiding this comment.
So, it has been Stream for a while, I think Stream was used to get the output to the console
| "-l", "com.dnsdock.image=outrigger", | ||
| "--name", "dnsdock", | ||
| "-p", bridgeIP + ":53:53/udp", | ||
| "-p", fmt.Sprintf("%s:53:53/udp", bridgeIP), |
| resolverReturn = cmd.configureMacResolver(machine) | ||
| } else if util.IsLinux() { | ||
| resolverReturn = cmd.configureLinuxResolver() | ||
| } else if util.IsWindows() { |
There was a problem hiding this comment.
It seems odd to have Linux in the middle, especially since it has a different signature.
There was a problem hiding this comment.
I tried to put them in order of use, meaning linux is 2nd most used.
|
|
||
| bip := strings.Trim(string(output), "\n") | ||
| if bip == "" { | ||
| bip = "172.17.0.1" |
There was a problem hiding this comment.
How do we know this bip is a safe fallback?
There was a problem hiding this comment.
172.17.0.1 is the default for Docker as configured by Docker. A user can change their default bridge IP.
tekante
left a comment
tekante
left a comment
There was a problem hiding this comment.
I didn't run any tests to confirm everything is working but I didn't see anything in the code that raised red flags for me. Presuming it's safe to leave in the DNS config for Linux even after things are stopped similar to Mac. If not a need to do the appropriate cleanup was the only thing that came to mind.
|
Since all of these configs target the .vm Ltd directly, I don’t see any harm in leaving them. We could do a quick blanket remove with an uninstall command or as part of the remove command or flag on the dns command but not sure it is worth it. Maybe as a follow up at some point later? |
|
Worth noting that currently we dont remove the mac settings either |
|
I'm not worried about uninstallation of this. I'd rather direct the time to #5 so we can more effectively help people understand the changes we are making. |
grayside
left a comment
grayside
left a comment
There was a problem hiding this comment.
I have some trouble keeping routes & resolvers clear in my head as dictionary terms, and the read flow is a bit rough, but overall the granularity of functions looks right and it's an improvement.
…ogging * origin/develop: Fixed bug in libnss-resolver DNS resolution (#116) Added rpm to build, well, rpms Transitioned to different go base to try to head off problems with dynamically linked go binaries tweaking some build flags tweaking CGO messing with ldflags tweaked goreleaser config fixed BaseCommand initialization in delegated commands (#114) Addeed .DS_Store to .gitignore Linux DNS install (#112)
3 participants
No description provided.