Update the pipectl and gRPC api to deal with event #1385
Conversation
|
Code coverage for golang is
|
|
/lgtm |
| // The data of event. | ||
| string data = 3 [(validate.rules).string.min_len = 3]; | ||
| // The ID of the project this event belongs to. | ||
| string project_id = 4 [(validate.rules).string.min_len = 1]; |
There was a problem hiding this comment.
I'm wondering if we really don't have to let Event have repo_id. Currently within a project, all events can be affected all repositories. So it can happen something like below:
Suppose this EventWatcher file is placed at repo1 as well as repo2:
apiVersion: pipecd.dev/v1beta1
kind: EventWatcher
spec:
events:
- name: app1-image-push
...Then you register a new event with the name app1-image-push.
pipectl event register \
--address=host.xz:443 \
--api-key=xxxx
--event-name=app1-image-push \
--data=foo/bar:v0.2.0
No matter what you intend to update which repository, both repo1 and repo2 will be updated. I figured out we have to prevent it in some way.
There was a problem hiding this comment.
One way we can solve this with the current design is, we denote users to name the event with the repo prefix like: repo1/app1-image-push. We can leave the responsibility to distinguish them to users by doing this.
There was a problem hiding this comment.
One way to make the event become more usable is that we can add tags/labels field to it.
EventWatcher or anything else can rely on that value.
There was a problem hiding this comment.
By that way, the event's generality is not reduced. And in the future, we can add another tag/label not even repo id.
There was a problem hiding this comment.
Totally agree with it. Thanks for the great suggestion, and yeah, we must not forget keeping up EventWatcher versatile, general and flexible definitely!
|
Thank you. Looks great! |
|
I have just added the labels field into the model. |
|
👍 |
|
🚀 |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: