-
Notifications
You must be signed in to change notification settings - Fork 599
bake: extra-hosts support #3234
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
Conversation
50e957c to
5370096
Compare
bake/bake.go
Outdated
| Ulimits []string `json:"ulimits,omitempty" hcl:"ulimits,optional" cty:"ulimits"` | ||
| Call *string `json:"call,omitempty" hcl:"call,optional" cty:"call"` | ||
| Entitlements []string `json:"entitlements,omitempty" hcl:"entitlements,optional" cty:"entitlements"` | ||
| ExtraHosts []string `json:"extra-hosts,omitempty" hcl:"extra-hosts,optional" cty:"extra-hosts"` |
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 opted for extra-hosts instead of add-hosts in Bake. Compose has also the same name.
tests/bake.go
Outdated
| `) | ||
| bakefile := []byte(` | ||
| target "default" { | ||
| extra-hosts = ["myhost=1.2.3.4"] |
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.
shouldn't this be map?
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.
changed to a map
Signed-off-by: CrazyMax <[email protected]>
| Ulimits []string `json:"ulimits,omitempty" hcl:"ulimits,optional" cty:"ulimits"` | ||
| Call *string `json:"call,omitempty" hcl:"call,optional" cty:"call"` | ||
| Entitlements []string `json:"entitlements,omitempty" hcl:"entitlements,optional" cty:"entitlements"` | ||
| ExtraHosts map[string]*string `json:"extra-hosts,omitempty" hcl:"extra-hosts,optional" cty:"extra-hosts"` |
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.
a single host name can actually have multiple IPs assigned (typically, to support IPv4 and IPv6)
For this purpose, Compose's HostsList is a map[string][]string. Maybe simpler to keep using a plain []string to mimic --add-host build flag
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.
Ah indeed this would not work with current logic. I guess with map[string]*string it would be smth like myhost = "8.8.8.8,::1" to have both v4/v6 ?
On LLB side I don't think we support that format though 🤔
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.
Better align with LLB model then, to avoid yet another undocumented format that will require additional parsing.
IMHO []string is fine as an API, as this naturally matches with --add-host on the CLI
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.
PR looks good but I don't have an opinion on the multiple IP thing.
|
Let's check the multiple-ip case for rc2. |
|
@tonistiigi doing so, there's a risk json model changes, isn't it? => compatibility break with Docker Compose to generate bake.json |
fixes #494