added cloud build event type

[capri-xiyue]

added cloud build event and data proto

[capri-xiyue]

assign @yolocs

[yolocs]

Do we only have one event type? In general the type should look like something google.cloud.build.something.v1.happened

[capri-xiyue]

Yep. Currently we only have one event type for cloudbuildsource

[yolocs]

@hrasadi

[capri-xiyue]

@nachocano

[hrasadi]

Overall LGTM. In terms of the event type string, is there a way for us to double check with the the service owners?

[capri-xiyue]

Overall LGTM. In terms of the event type string, is there a way for us to double check with the the service owners?

@hrasadi When you say event type string, do you mean this one google.cloud.build.status.v1.changed? What do you mean by double check with the the service owners? I think I'm gonna do some change for this file https://github.com/google/knative-gcp/blob/b3d705927b0842a190661df14936aefdd22942ad/config/core/resources/cloudbuildsource.yaml#L25 to make it match? Is that what you mean for the double check?

[hrasadi]

I mean if possible, asking the Cloud Build team what the event type should be called.

[capri-xiyue]

I mean if possible, asking the Cloud Build team what the event type should be called.

I refer to this https://cloud.google.com/cloud-build/docs/subscribe-build-notifications to define the naming of event type.
Cloud Build publishes messages on a Google Pub/Sub topic when your build's state changes, such as when your build is created, when your build transitions to a working state, and when your build completes.
I will aslo consult Cloud Build team to see whether they have any insights.

[yolocs]

FWIW, the actual cloud build resource is called build. According to the current convention, the event type should be something like google.cloud.build.build.v1.statusChanged. But the double "build" might cause the illusion it's a mistake.

We should consult cloud build team for sure. An alternative could be google.cloud.build.buildstatus.v1.changed. But it doesn't follow the existing convention strictly.

@jskeet @hrasadi any thoughts?

[jskeet]

Hmm. I think I prefer the odd-looking google.cloud.build.build.v1.statusChanged event type. I think the consistency of that will help with consistency in others aspects which don't have the oddity, whereas if we decide to break the convention for this aspect of Cloud Build, it might make some other aspects less conventional as a side-effect. But I'll readily admit I haven't put much time/thought into it.

[capri-xiyue]

@jskeet @hrasadi @yolocs
I just got reply from Cloud Build Team,
I would have expected "google.cloudbuild.v1.status.changed" or possibly "google.cloudbuild.v1.build.status.changed"? Meaning effectively (in my head) that the service is cloudbuild.v1, the object in question is a build, the field in question is build.status, and the event is that the status changed. But as usual, naming is hard...
But what Cloud Build eam suggests doesn't follow our convention.
For me I prefer google.cloud.build.build.v1.statusChanged since it follows our convention.

[jskeet]

Could I suggest that we take this discussion internal at this point? I think it'll probably be simpler.

[capri-xiyue]

Could I suggest that we take this discussion internal at this point? I think it'll probably be simpler.

sure

[jskeet]

For a "gold standard" experience, we would rewrite all the field descriptions from the perspective of "this is a build that has already been created", but that's probably overkill. I've noted a few places where it really would improve things to move away from the "the user might be providing this" model.

[yolocs]

/lgtm

[jskeet]

Okay, this looks fine. (I have quibbles about a few aspects of the proto design, but that's a ship that sailed long ago.)

We may come back and ask for sample event data to validate the schema against once #52 is in.