Conversation
6af7693 to
b010238
Compare
Code Coverage
|
3eb0a7e to
46bd18b
Compare
|
Thanks for the review, @katiewasnothere. Happy to iterate further if anything else needs addressing. |
|
Hi @0xMH thank you for all the work you've done on this change! We are planning to make a new release of the container package some time in the next week and we want to hold off on merging this change until then. |
|
Hi @0xMH and @katiewasnothere! First, thanks for the great work figuring out the core logic for this. I noticed this PR has been stalled since the June release and has picked up some merge conflicts. I also saw that a couple of newer PRs for #1449 were opened recently, but they seem to have missed the @0xMH, if you are currently swamped with other things, would you mind if I pull your branch, resolve the conflicts against Let me know if that sounds good! |
|
Thanks for the offer! but no need since I'm waiting for the green flag to continue and rebase and prepare for the merge. |
|
Awesome, sounds good! Looking forward to seeing it merged. Let me know if you end up needing a hand later on! |
|
@0xMH Really sorry for the delay here, we've been swamped. I'd love to get this in for this sprint (which ends at the end of August). Could you rebase your PR? I will take another pass through the code. Thank you for your work here! |
| - `192.168.*.*` | ||
| - `172.16.*.*` through `172.31.*.*` | ||
| - The host ends with the machine's default container DNS domain (as defined in `DNSConfig.defaultDomain`, located [here](../Sources/ContainerPersistence/ContainerSystemConfig.swift)) | ||
| - The host ends with the machine's configured internal DNS domain from `[dns].domain` |
There was a problem hiding this comment.
Could we add a link to the reference doc here instead?
| public static func dnsConfiguration( | ||
| from flags: Flags.DNS, | ||
| defaults: ContainerDNSConfig, | ||
| hostDomainFallback: String? = nil |
There was a problem hiding this comment.
This to preserve the existing behavior. hostDomainFallback uses [dns].domain only when no --dns-domain flag or [container.dns].domain is set. I added tests for the fallback and for container specific override
| public let nameservers: [String] | ||
| public let searchDomains: [String] | ||
| public let options: [String] |
There was a problem hiding this comment.
What do you think about making these optionals instead of empty arrays?
There was a problem hiding this comment.
Yes, I made them optional.
|
|
||
| public let cpus: Int | ||
| public let memory: MemorySize | ||
| public let dns: ContainerDNSConfig |
There was a problem hiding this comment.
What do you think about making this an optional as well?
There was a problem hiding this comment.
Yes, I made ContainerConfig.dns optional as well.
|
@0xMH I'm Scrubbing through some of the PRs that fell through the cracks. When you can, can you rebase and address the PR review feedback? Thanks! |
46bd18b to
606c4d4
Compare
606c4d4 to
5b9e784
Compare
|
I rebased the branch and addressed the comments. Please let me know if there’s anything else to address. |
This branch has not been deployed
4 participants
Type of Change
Motivation and Context
Closes #1449. Adds default values for
--dns,--dns-search,--dns-option, and--dns-domainto the[dns]section of~/.config/container/config.toml, so users hitting macOSmDNSResponderconflicts can set the workaround once instead of repeating it on every invocation. Depends on the merged TOML configuration introduced by #1425.Defaults are read by
container run,container build, andcontainer builder startvia a sharedUtility.dnsConfiguration(from:defaults:)helper. CLI flags take precedence;--no-dnsstill disables DNS. Two pre-existing bugs in the build path were fixed to make the feature work end-to-end:BuildCommandwas forwarding onlydnsNameserversto the builder (now forwards all four DNS fields), andBuilderStart'sdnsChangedcheck only compared the first non-empty field (now compares all four).Testing