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

es_verify_cert is expected to be a lowercase string #259

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

sarahbx
Copy link
Contributor

@sarahbx sarahbx commented Mar 17, 2021

I was going to open an issue with benchmark-operator, but I feel the fix belongs in the wrapper. After updating my local benchmark-operator repo, I have been unable to connect to my self-signed Elasticsearch instance.

Recently the Benchmark CRD elasticsearch.verify_cert field was set to a boolean. It was previously a string to support how the benchmark-wrapper handles it. Because of this, when verify_cert is set and added to templates, the string becomes capitalized during the python boolean to string conversion.

To fix this in the benchmark-operator, to make sure it always output a lowercase string can be fragile when there are so many templates to maintain and the ability to Bring-Your-Own-Workload.

Instead, I propose we make benchmark-wrapper more forgiving of env input, at a minimum converting the relevant input to lowercase.

Copy link
Member

@jtaleric jtaleric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@comet-perf-ci
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Member

@rsevilla87 rsevilla87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @sarahbx , LGTM!

@rsevilla87 rsevilla87 merged commit c8cda5d into cloud-bulldozer:master Mar 18, 2021
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.

4 participants