search

google / slo-generator

SLO Generator computes SLIs, SLOs, Error Budgets and Burn Rates from supported backends, then exports an SLO report to supported targets.
Apache License 2.0
489 stars 78 forks source link

fix: make unit tests pass again with elasticsearch 8.x client #223

Closed lvaylet closed 2 years ago

lvaylet commented 2 years ago

Thanks for catching this. I reused your suggested code, slightly updated it to handle missing keys on dict.pop() and added comments to make maintenance easier later on.

Now does that mean that we should replace url with hosts in the configuration files? Version 8.x.x of the client requires either hosts or cloud_id to be specified.

Finally, should I write unit tests to cover all these scenarios (Elastic Cloud, basic auth, multiple nodes, API token...)?

ocervell commented 2 years ago

I think we should maintain the backward compat for this one, so let's keep url for now and we pass it as hosts to the client. We should update our Elasticsearch docs a bit (can be done in a further PR probably) to specify that url can be a single host (string) or a set of hosts (list).

About the unit tests, that's probably a good idea :) maybe split them in a further PR so that we fix the broken unit tests with this PR asap.

lvaylet commented 2 years ago

I'd rather not accumulate technical debt so I updated the docs in this PR. I created a separate issue for the unit tests: https://github.com/google/slo-generator/issues/227.

lvaylet commented 2 years ago

Thanks @ocervell for approving the changes. Do you expect me to go ahead and merge the PR myself? If so, which method should I use: "merge commit" or "squash and merge" (my preference)? If "squash and merge", are you happy with the commit message (i.e. the PR title)?