tiltfile: can attach links to local resource [ch9609] #3833
Conversation
internal/tiltfile/links.go
Outdated
| "github.com/tilt-dev/tilt/pkg/model" | ||
| ) | ||
|
|
||
| func convertLinks(val starlark.Value) ([]model.Link, error) { |
There was a problem hiding this comment.
can you make this implement the Unpack interface? here's a good example
https://github.com/tilt-dev/tilt/blob/master/internal/tiltfile/value/duration.go#L21
internal/tiltfile/links.go
Outdated
| } | ||
| } | ||
|
|
||
| func (s *tiltfileState) link(thread *starlark.Thread, fn *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { |
There was a problem hiding this comment.
can you put this in an extension? i don't have strong opinions about which extension it is. just would like to keep the tiltfileState kitchen sink from growing
There was a problem hiding this comment.
I mean theoretically yes? but it feels weird to me to hide what the constructor of what is theoretically a first class Tiltfile object in an extension. I feel like we should be encouraging everyone to uses local_resource.links to name their links for self-documenting-ness, so the less friction we have there, the better. wdyt?
There was a problem hiding this comment.
i think you might be misunderstanding my comment. local_resource should eventually go in an extension too. how the code is organized shouldn't change the syntax.
There was a problem hiding this comment.
oh whoops yeah -- I thought you meant an extension
internal/tiltfile/local_resource.go
Outdated
| @@ -87,6 +90,11 @@ func (s *tiltfileState) localResource(thread *starlark.Thread, fn *starlark.Buil | |||
| return nil, fmt.Errorf("local_resource must have a cmd and/or a serve_cmd, but both were empty") | |||
| } | |||
|
|
|||
| links, err := convertLinks(linksVal) | |||
There was a problem hiding this comment.
once this is an Unpacker, this will go away (since starlark will be able to correctly identify which argument failed to parse)
internal/tiltfile/links.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| type link struct { |
There was a problem hiding this comment.
should we embed a Starlark Struct for this?
https://pkg.go.dev/go.starlark.net/starlarkstruct#Struct
e.g.,
type link struct {
Struct
model.Link
}
Struct comes out of the box with a bunch of semantics that make sense for this value, rather than hand-implementing Truth(), Hash(), etc yourself.
There was a problem hiding this comment.
ooh neat, I was just cribbing from portForward that sgtm
There was a problem hiding this comment.
hm do we have an example of this somewhere in the code? (I can't see one.) i'm running into some trouble with pointer receivers and i'm going to leave this for now.
Hello @nicks, @landism,
Please review the following commits I made in branch maiamcc/local-resource-link-tiltfile:
047d4ba (2020-10-06 12:33:42 -0400)
clean up
01f30a2 (2020-10-05 18:35:42 -0400)
implement, tests green
b3eef22 (2020-10-05 12:56:25 -0400)
red test
Code review reminders, by giving a LGTM you attest that: