webview: add buildResult.BuildType to webview proto [ch4520] #2956
Conversation
pkg/webview/view.proto
Outdated
| @@ -22,6 +22,15 @@ message BuildRecord { | |||
|
|
|||
| // The span id for this build record's logs in the main logstore. | |||
| string span_id = 8; | |||
|
|
|||
| enum BuildType { | |||
There was a problem hiding this comment.
i would probably make this a top-level type rather than nesting it in BuildRecord
There was a problem hiding this comment.
...wait, can you elaborate on why you want this to be top-level? Now that I'm making UpdateType and TargetType into their own enums, I'm running into some name collisions b/c they're both top-level.
There was a problem hiding this comment.
This is a common question! The style guide has an answer! :)
https://developers.google.com/protocol-buffers/docs/style#enums
Prefer prefixing enum values instead of surrounding them in an enclosing message.
So it should be BUILD_TYPE_IMAGE
There was a problem hiding this comment.
🤔 yeah I saw that but once you generate the .pb file you get names like BuildType_BUILD_TYPE_IMAGE--but uh, if that's what the style guild says, I guess I'll go with it? (I thought that might be a guideline for like, the generated file as opposed to the .proto file? but, 🤷♀ )
There was a problem hiding this comment.
ya, there's a long discussion on it here
golang/protobuf#513
(and follow-up issues), the reasons are pretty complex and nuanced
pkg/webview/view.proto
Outdated
| @@ -8,20 +8,31 @@ import "pkg/webview/log.proto"; | |||
|
|
|||
| option go_package = "github.com/windmilleng/tilt/pkg/webview"; | |||
|
|
|||
| enum BuildType { | |||
| IMAGE = 0; | |||
There was a problem hiding this comment.
in protobufs, the 0 enum value is special, and is generally used for some sort of natural zero value or unspecified value. See:
https://developers.google.com/protocol-buffers/docs/style#enums
because it indicates the case where the field wasn't populated. I would not expect IMAGE to be a zero value
pkg/webview/view.proto
Outdated
| @@ -93,6 +104,7 @@ message Resource { | |||
| LocalResourceInfo local_resource_info = 17; | |||
| string runtime_status = 18; | |||
| bool is_tiltfile = 19; | |||
| repeated BuildType target_types = 27; | |||
There was a problem hiding this comment.
hmmmmm...build types and target types are different enums in our Go data model. Treating them as the same here is going to lead to a lot of confusion.
Can we make this more closely match the Go data model? maybe something like:
repeated TargetSpec specs;
message TargetSpec {
name string
type string
has_live_update bool
}
maiamcc
force-pushed
the
maiamcc/webview-live-update-info
branch
from
41d7953 to
b6f093f
Compare
maiamcc
force-pushed
the
maiamcc/webview-live-update-info
branch
from
b6f093f to
b11c775
Compare
maiamcc
force-pushed
the
maiamcc/webview-live-update-info
branch
from
50b44f1 to
f4992c4
Compare
| } | ||
|
|
||
| // Correspond to implementations of the TargetSpec interface | ||
| enum TargetType { |
There was a problem hiding this comment.
IDK what the most idiomatic way is to name these such that I avoid name conflicts (i can't have two enums in this package both named "image" e.g.). Or is the solution to have one (or both) of these types be nested? (I share your "ehh this should be top level" feeling but it's just a gut thing for me, idk your reasoning.)
| } | ||
|
|
||
| func TestBuildHistory(t *testing.T) { |
There was a problem hiding this comment.
wanted test coverage for my change, but unsure if this is too finicky, thoughts?
There was a problem hiding this comment.
this is fine, tho it might be worthwhile to eventually invest in a kind of golden test around this
| } | ||
|
|
||
| func TestBuildHistory(t *testing.T) { |
There was a problem hiding this comment.
this is fine, tho it might be worthwhile to eventually invest in a kind of golden test around this
another WIP branch for you -- this one adds BuildResult.BuildTypes to the proto
so that you can access it from the tsx. It'll be a list of enums that you can
look thru to see if there's a
LiveUpdatepresentagain, sorry for the mess, lack of tests, etc. 😬