Add Event key to identify events #1440
Conversation
|
Code coverage for golang is
|
| // This is intended to be used to specify additional attributes of event. | ||
| map<string,string> labels = 5; | ||
| // A unique identifier consists of its own name and labels. | ||
| string event_key = 6 [(validate.rules).string.min_len = 1]; |
There was a problem hiding this comment.
Where is the hash value that we talked about before?
There was a problem hiding this comment.
My apologies, I totally forgot it.
There was a problem hiding this comment.
Thank you. I've done added the hash just now.
|
Code coverage for golang is
|
| func (s *store) GetLatest(ctx context.Context, name string, labels map[string]string) (*model.Event, bool) { | ||
| key := makeEventKey(name, labels) | ||
| s.mu.RLock() | ||
| key := model.MakeEventKey(name, labels) |
There was a problem hiding this comment.
nit: This should be done before locking.
| // Check if we can get the exact same string. | ||
| got1 := MakeEventKey(tc.name1, tc.labels1) | ||
| got2 := MakeEventKey(tc.name2, tc.labels2) | ||
| assert.Equal(t, tc.wantSame, got1 == got2) |
There was a problem hiding this comment.
Add the check of key length to ensure that they are not empty strings.
pkg/datastore/eventstore.go
Outdated
| } | ||
|
|
||
| func (s *eventStore) AddEvent(ctx context.Context, e model.Event) error { | ||
| e.EventKey = model.MakeEventKey(e.Name, e.Labels) |
There was a problem hiding this comment.
I think it is better to set the key when making the event instead of here.
https://github.com/pipe-cd/pipe/blob/master/pkg/app/api/grpcapi/api.go#L323
There was a problem hiding this comment.
Every event should have an event-key derived from the exact same method. So I thought making here is more appropriate, but you don't think so right?
There was a problem hiding this comment.
Yes, I got your point. I was thinking the same.
But I feel updating the model inside this function is not good because the store is just for storing and querying.
(To be honest, I also want to remove the part of updating CreatedAt and UpdatedAt from this function. But that is future talk.)
There was a problem hiding this comment.
Fine, that makes total sense. This datastore package provides non-domain-specific, versatile facilities and should just operate external Datastore services. I feel exactly the same, so let me fix it!
|
I've fixed all of those pointed out! |
|
Code coverage for golang is
|
|
/lgtm |
|
🚀 |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
After merging this, it will:
Which issue(s) this PR fixes:
Fixes #1421
Fixes #1437
Does this PR introduce a user-facing change?: