octodns / octodns-constellix

Constellix DNS provider for octoDNS
MIT License
1 stars 4 forks source link

Cleanup healthcheck handling #35

Closed istr closed 9 months ago

istr commented 1 year ago

This PR implements full SUPPORTS_POOL_VALUE_STATUS capability and creates checks by default as per documentation. https://github.com/octodns/octodns/blob/main/docs/dynamic_records.md

Fixes #39

ross commented 1 year ago

Probably won't have a chance to look in detail until early next week, but if you all complete it and review w/o me that's fine. My only high level input is that we'll need to be careful with the upgrade/downgrade paths and make sure that at the very least the provider doesn't get things into a broken/stuck state. It can make changes at the upgrade/downgrade if it has to. If it does so then we'd want to call that out clearly in the CHANGELOG.

istr commented 1 year ago

Some of my initial thoughts without having familiarity with this codebase.

@viranch thanks a lot.

To be honest I would completely ditch the homegrown caching here and replace it with a request_cache based solution.

istr commented 1 year ago

Probably won't have a chance to look in detail until early next week, but if you all complete it and review w/o me that's fine.

That is a good timing. Lots of the old code still needs verification against the real API from my side.

For me there seems to be a lot of code that was never actually used or only worked by chance.

For example, updates using PUT never return the new record in the constellix API. But the code assumes so for some objects and reassigns data from results that can only contain a success string. So for example the old code for pool_update or geofilter_update simply couldn't work as expected against the real constellix API.

My only high level input is that we'll need to be careful with the upgrade/downgrade paths and make sure that at the very least the provider doesn't get things into a broken/stuck state. It can make changes at the upgrade/downgrade if it has to. If it does so then we'd want to call that out clearly in the CHANGELOG.

From my perspective, the one major upgrade would be that the sensors will be created / enabled by default as advertised in the documentation. Currently, the constellix provider only activates monitoring if the provider specific configuration entry is in place.

So it would be worth mentioning in the changelog

istr commented 1 year ago

@ross

If you're in contact with them, looping them in for their thoughts on the changes/potential impact would probably be a good idea.

Indeed. So here you go, @davidmmiller, @stevehockey, @jr-frazier - I thought you might be interested in what is going on here.