story(notifications): support apache kafka #55
Conversation
Zaba505
added
enhancement
|
|
||
| // Bus | ||
| type Bus interface { | ||
| // Publish |
| ErrMissingLogger = errors.New("logger is required") | ||
| ) | ||
|
|
||
| // Bus |
| Cause error | ||
| } | ||
|
|
||
| func (e Error) Error() string { |
There was a problem hiding this comment.
need comment for exported function
| return fmt.Sprintf("%s: encountered unexpected error: %s", e.Bus, e.Cause) | ||
| } | ||
|
|
||
| func (e Error) Unwrap() error { |
There was a problem hiding this comment.
need comment for exported function
| Cause error | ||
| } | ||
|
|
||
| func (e MarshalError) Error() string { |
There was a problem hiding this comment.
need comment for exported function
| } | ||
|
|
||
| var ( | ||
| ErrMissingKafkaAddrs = errors.New("kafka addresses are required") |
notification/kafka.go
Outdated
| panic(ConfigurationError{ | ||
| Bus: kafkaBus, | ||
| Cause: err, | ||
| }) |
There was a problem hiding this comment.
panic bad, return an error instead of panicking
| return nil | ||
| } | ||
|
|
||
| // Publish |
| type KafkaConfig struct { | ||
| Addresses []string | ||
| AllowAutoTopicCreation bool | ||
| Logger *zap.Logger |
There was a problem hiding this comment.
why provide the logger via the config. Wouldn't it be better to use the global zap for consistency and just add required fields for kafka?
There was a problem hiding this comment.
Global state should always be avoided. Only the cmd package is using the global logger and that's simply cuz I haven't updated it yet to not use the global one.
| Addresses []string | ||
| AllowAutoTopicCreation bool |
There was a problem hiding this comment.
add validator tags and use the validator for this
|
No longer going this route |
Zaba505
deleted the
story/notifications/issue-24-support-apache-kafka
branch
Closes #24