-
Notifications
You must be signed in to change notification settings - Fork 558
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
@@ -726,6 +726,7 @@ type Target struct { | |||
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 <1951866+crazy-max@users.noreply.github.com>
@@ -726,6 +727,7 @@ type Target struct { | |||
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