tiltfile: rejigger live update declaration syntax"#1433
Conversation
| case LiveUpdateSyncStep: | ||
| if seenRunStep { | ||
| return LiveUpdate{}, errors.New("all sync steps must precede all run steps") | ||
| return LiveUpdate{}, errors.New("live_update: all sync steps must precede all run steps") |
There was a problem hiding this comment.
it seems a bit cleaner to leave the live_update off here and attach it in the tiltfile instead, so that function names are all in that package, but nbd
| // just another step INTERNALLY too (instead of as a non-step property of a LiveUpdate). | ||
| if fallBackStep, ok := ms.(model.LiveUpdateFallBackOnStep); ok { | ||
| if len(fallBackOn) != 0 { | ||
| return model.LiveUpdate{}, fmt.Errorf("live_update: cannot specify more than one "+ |
There was a problem hiding this comment.
I was surprised by this and wondered why we only support specifying one path to fall back on!
After a few seconds' thought, I realized fall_back_on probably takes a union of str or list[str]. Maybe worth some text here like "just pass all paths to a single step!"?
(or maybe we should support having multiple? that might look cleaner than a long list of strings in a single step?)
There was a problem hiding this comment.
ehh, supporting multiple is gonna get weird when actually store them as steps on the model.LiveUpdate. I'll fix the error message.
There was a problem hiding this comment.
actually we do the same thing for adds vs runs don't we. kk
|
|
||
| for k, v := range s.liveUpdates { | ||
| if !v.matched { | ||
| return fmt.Errorf("live_update was specified for '%s', but no built resource uses that image", k) |
| `, codeToInsert) | ||
| f.file("Tiltfile", tiltfile) | ||
|
|
||
| fmt.Println(tiltfile) |
There was a problem hiding this comment.
if this is intentional rather than temporary debug, it might be better to fmt.Printf("tiltfile:\n%q\n", tiltfile)
| func (s *tiltfileState) consumeLiveUpdateStep(stepToConsume liveUpdateStep) { | ||
| for i, step := range s.unconsumedLiveUpdateSteps { | ||
| if step.declarationPosition() == stepToConsume.declarationPosition() { | ||
| copy(s.unconsumedLiveUpdateSteps[i:], s.unconsumedLiveUpdateSteps[i+1:]) |
There was a problem hiding this comment.
usually when people use this idiom, they want to zero out the value at the end of the slice?
or did you intend to use a simpler idiom like
s.unconsumedLiveUpdateSteps = append(s.unconsumedLiveUpdateSteps[i:], s.unconsumedLiveUpdateSteps[i+1:]...)
?
| return starlark.None, nil | ||
| func (s *tiltfileState) consumeLiveUpdateStep(stepToConsume liveUpdateStep) { | ||
| for i, step := range s.unconsumedLiveUpdateSteps { | ||
| if step.declarationPosition() == stepToConsume.declarationPosition() { |
There was a problem hiding this comment.
fwiw, i'm not at all convinced that this equality check will behave like you expect. it has unexported pointer fields. and the struct might change a lot in the future
it might be more robust to create your own struct with the equality semantics you want, and use it as a map key to do inserts/deletes
3 participants
Hello @landism, @nicks,
Please review the following commits I made in branch maiamcc/live-update-syntax:
13940be (2019-04-04 14:09:50 -0400)
integration test
f8092c6 (2019-04-04 14:02:04 -0400)
custom build support
de1c755 (2019-04-04 13:58:09 -0400)
fall_back_on works
0a28d00 (2019-04-04 12:54:30 -0400)
more red tests - ready for fall_back_on
24608d8 (2019-04-04 12:47:01 -0400)
tests so far mostly green
d5fcf8f (2019-04-04 12:22:30 -0400)
red tests
Code review reminders, by giving a LGTM you attest that: