-
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
Matcher v2 init #541
Matcher v2 init #541
Conversation
Hard-Coded Secrets (1)
More info on how to fix Hard-Coded Secrets in General. Insecure Network Communication (1)
More info on how to fix Insecure Network Communication in Go. Vulnerable Libraries (1)
More info on how to fix Vulnerable Libraries in Go. 👉 Go to the dashboard for detailed results. 📥 Happy? Share your feedback with us. |
} | ||
|
||
// TODO: this is a big reason why I want to refactor the Matcher logic | ||
func (m2 *MatcherV2) getUrls() (urls *ring.Ring) { |
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 don't believe MatcherV2 is going to need getUrls - in my refactored branch i move the urls to the matcher to get rid of this function which is also called here:
caduceus/internal/sink/sinkSender.go
Line 423 in dad38f9
urls = s.matcher.getUrls() |
|
||
config.Producer.Return.Successes = true |
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 added back in the default config because it kept failing due to certain fields not being set. i'm setting these fields this way based on a tutorial I found: https://youtu.be/j6bqJKxb2w0?si=-htt-I0kgF8RqHMy&t=1384
// Create a new Kafka producer | ||
producer, err := sarama.NewSyncProducer(kafka.brokerAddr, config) | ||
if err != nil { | ||
kafka.logger.Error("Could not create Kafka producer", zap.Error(err)) | ||
return nil | ||
} | ||
defer producer.Close() |
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.
@@ -68,31 +67,32 @@ func NewSink(c Config, logger *zap.Logger, listener ancla.Register) Sink { | |||
kafka := &Kafka{ | |||
id: l.Registration.CanonicalName, | |||
brokerAddr: k.BootstrapServers, | |||
topic: "test", | |||
topic: "quickstart-events", |
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.
this is based off the kafka docs for testing locally... i imagine we will be including topic as a part of the kafka webhook struct that users will be sending us?
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.
yup
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.
from the title it sounds like this is just laying out the ground work and it's not the final version of matcher v2.
in that case, lgtm!
What's Included: