This message was deleted.

I added a comment to pr about a potential additional change. But also I'm running on 1.25 right now, what about the version of fabric needed updating? Everything seems okay to me, so far.

its unsupported for k8s 1.25, I also mentioned in the comments of the exact issue people experienced in k8s https://github.com/apache/druid/pull/13804#discussion_r1112268989

I ported your changes back to my druid and they worked. But I didn't require the fabric bump in version. I guess it doesn't really matter, doesn't hurt.

i don’t have a 1.25 cluster to test on, this is just debugging with some folks that were running this on 1.25, it makes sense from the documentation that the fabric client wouldn’t work as the way k8s tells you a job is complete has changed.

Got it.

i'm not familiar with k8s but i can review from a generic project perspective— is that useful? (i would be trusting you that the k8s-specific stuff is good 🙂)

it looks good to me. Though the code coverage check is failing because of lack of tests for DruidKubernetesClient class. And you have a dummy test specifically added to make code coverage happy.. If coverage is still failing, we don't need that dummy test either. Let me know if I missed something

that is not really a dummy test, it tests a getter i don;’t know how useful the test is, but it does “make code coverage happy”. The thing was that was making code coverage unhappy was the k8s client changed how they do deletes, they used to return a boolean, now it returns a list of objects and if that list is empty, the logic is the same as before. Now the code has already been tested, but the code coverage tool thinks the logic has changed, but it hasn’t the client behavior is just different. Don’t know how to test that and keep it useful.

since the code coverage is failing anyway, I suggest that this method be deleted. If the test has a purpose beyond making code coverage happy, then it should be renamed as such. When I see a test named "makeCodeCoverageHappy", I assume that its sole purpose is to get coverage passing.

you are right, it is meaningless, ill remove it and push up

@Abhishek Agarwal did you mean to ✅ this one? @churro — it looks like some conflicts crept in, mind fixing them?

@Gian Merlino - will do once that test is removed.

test is removed, sorry it took so long