search

scylladb / scylla-machine-image

Apache License 2.0
18 stars 25 forks source link

lib/scylla_cloud: use functool.cache to cache identify_cloud_async() #514

Closed tchaikov closed 3 months ago

tchaikov commented 3 months ago

this change is a follow-up of d97e311130, which caches the result of identify_cloud_async() using a global variable. it would be more idiomatic to use functool.cache for caching a costy function like identify_cloud_async(), so in this change we use it for caching. simpler this way. and more importantly, it is more readable.

yaronkaikov commented 3 months ago

@tchaikov It seems that this change is breaking tests. please check it as well https://github.com/scylladb/scylla-machine-image/actions/runs/8447372797/job/23137618753?pr=514

tchaikov commented 3 months ago

@tchaikov It seems that this change is breaking tests. please check it as well https://github.com/scylladb/scylla-machine-image/actions/runs/8447372797/job/23137618753?pr=514

it does. so we are testing w/ python 3.8.18, while functools.cache was introduced in 3.9! see https://docs.python.org/3/library/functools.html#functools.cache

tchaikov commented 3 months ago

@fruch hi Israel, do we need to support python 3.8? on RHEL8 python 3.6 is the default. so we are not likely to be stuck with RHEL8. in RHEL9, Python 3.9 is the default. Fedora 38, 3.11. ubuntu 22.04 3.10. but yeah, ubuntu 20.04, 3.8. so are we trying to be compatible with ubuntu 20.04?

fruch commented 3 months ago

@fruch hi Israel, do we need to support python 3.8? on RHEL8 python 3.6 is the default. so we are not likely to be stuck with RHEL8. in RHEL9, Python 3.9 is the default. Fedora 38, 3.11. ubuntu 22.04 3.10. but yeah, ubuntu 20.04, 3.8. so are we trying to be compatible with ubuntu 20.04?

None of the above, we are using our own python version from scylla-python3, and currently on master it's 3.11 (if I remember correctly)

tchaikov commented 3 months ago

@fruch thanks for confirmation. lemme bump the github workflow to use Python 3.11 then. that should match with the version we use in production.

tchaikov commented 3 months ago

pending on #515

tchaikov commented 3 months ago

@fruch good to merge?

fruch commented 3 months ago

@fruch good to merge?

I think yes

but leave it to @yaronkaikov to take it further (and if needed backport or not)

tchaikov commented 3 months ago

ack. it's more of a cleanup. so IMHO, no need to backport.