Skip to content
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

check if certificates is empty strings #105

Merged
merged 9 commits into from
Jun 26, 2018
Merged

check if certificates is empty strings #105

merged 9 commits into from
Jun 26, 2018

Conversation

e-max
Copy link
Contributor

@e-max e-max commented Jun 26, 2018

Kafka driver enable TLS connection if user passed key and certificate. Before we consider any certificate valid if it wasn't nil. In this PR we check if it also not empty string

@e-max e-max requested a review from 2color June 26, 2018 15:34
@dgellow
Copy link
Contributor

dgellow commented Jun 26, 2018

@e-max What about using a string instead of a []byte, that we then only cast when needed (i.e: just before the call to newTLSConnection, in the case where we want to use TLS)?

@dgellow
Copy link
Contributor

dgellow commented Jun 26, 2018

That way the caller doesn't have to cast anything and can directly pass a value from os.Getenv.

dgellow
dgellow previously approved these changes Jun 26, 2018
@dgellow
Copy link
Contributor

dgellow commented Jun 26, 2018

Also, do we need tests for this change?

@dgellow
Copy link
Contributor

dgellow commented Jun 26, 2018

Why is the update of certificates in tests necessary?

Edit, based on slack exchange:

tests were failing because our tests certificates had expired. I've updated them in the same branch

@2color 2color merged commit b5e5fe6 into master Jun 26, 2018
@dgellow dgellow deleted the check_certificates branch June 27, 2018 08:12
@e-max
Copy link
Contributor Author

e-max commented Jul 2, 2018

arr. Guthub stopped to send me an email again! I missed all your comments @dgellow . Sorry about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants