-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hashing Logic #556
Hashing Logic #556
Conversation
@@ -91,8 +102,7 @@ func NewSink(c Config, logger *zap.Logger, listener ancla.Register) Sink { | |||
} | |||
|
|||
func (v1 *WebhookV1) Update(c Config, l *zap.Logger, altUrls []string, id, failureUrl, receiverUrl string) (err error) { | |||
//TODO: is there anything else that needs to be done for this? | |||
//do we need to return an error | |||
//TODO: do we need to return an error if not - we should get rid of the error return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we don't need it, then we need to see whether the error fulfills any interfaces that we depend on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work as always 💯
I have a few suggestions 🙂
@@ -51,9 +51,10 @@ _testmain.go | |||
.vscode/* | |||
.dev/* | |||
|
|||
caduceus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wes had me move the main.go file from cmd/main.go to cmd/caduceus/main.go for correct functionality of goschtalt - this line caused the main.go file to be ignored so added yaml so it would ignore changes to just the yaml file
removing file from this commit - not necessary. will add it back in later.
internal/sink/sink.go
Outdated
} | ||
|
||
} else { | ||
errs = fmt.Errorf("could not find kakfa for the related hash %v", k.HashField) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to send an error here? or instead of the k.HashField if check do we want say if err != nil we go straight to the for loop of all the kafkas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets leave this, but we need to think about how to best describe possible error cases like this, i.e. define errors, new metrics, logging (I think this section is good, just add a todo saying we need to flush out the error handling for kafka)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically hashing can't fail as long as there is 1 item so k.Kafkas[k.Hash.Get(GetKey(k.HashField, msg))]
should always be good
but to make sure of this, we can add a check len(k.Kafkas) == len(k.Hash)
during the Hash and kafka setup
err := kafka.send(secret, acceptType, msg) | ||
if err != nil { | ||
errs = errors.Join(errs, err) | ||
if len(*k.Hash) == len(k.Kafkas) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to the do this check differently, but we're ok to merge this for now and I'll patch it in a new pr
What's Included: