-
Notifications
You must be signed in to change notification settings - Fork 0
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
Improve env validation and definition #19
base: main
Are you sure you want to change the base?
Conversation
const useSSL = kafkaSecurityProtocol === 'SSL' | ||
const useSasl = env.get('KAFKA_SASL_MECHANISM') !== undefined | ||
|
||
const saslOptions = useSasl |
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.
not sure we want this in the default stub. I’d opt for getting folks going fast in a dev env. But we could leave this commented out so folks know they have this path for a live env
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.
Mmm, maybe. Idk.
KAFKA_REAUTHENTICATION_TIMEOUT: `Env.schema.number.optional()`, | ||
|
||
// We don't yet support AWS IAM or OAuth Bearer or custom mechanisms | ||
KAFKA_SECURITY_PROTOCOL: `Env.schema.enum.optional(['PLAIN', 'SSL'])`, |
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.
may want:
KAFKA_SECURITY_PROTOCOL: the security protocol the hashing service should use when communicating with
the Kafka cluster. Can be set to ssl, plaintext, or sasl_plaintext and defaults to plaintext.
This should resolve #3; I've added logic in to exclude configuration for the
aws
andoauthbearer
mechanisms, since we've not tested them and they can't necessarily be configured via environment variables. I'm not sure if this is a good idea or not?Based on #18