beacon-biosignals / K8sClusterManagers.jl

A Julia cluster manager for Kubernetes
Other
31 stars 5 forks source link

Refactor `isk8s` #99

Closed omus closed 1 year ago

omus commented 1 year ago

Noticed this isk8s function no longer works on the latest version of Kubernetes. I've refactored this function to work on modern versions and have added some in-cluster tests to ensure this isn't missed again if this functionality breaks again.

codecov[bot] commented 1 year ago

Codecov Report

Merging #99 (b17358f) into main (22db94b) will decrease coverage by 2.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
- Coverage   66.86%   64.81%   -2.05%     
==========================================
  Files           4        4              
  Lines         172      162      -10     
==========================================
- Hits          115      105      -10     
  Misses         57       57              
Impacted Files Coverage Δ
src/pod.jl 97.59% <100.00%> (-0.26%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

omus commented 1 year ago

Any reason not to also check /var/run/secrets/kubernetes.io/serviceaccount/token as described in the docs?

The referenced documentation is for now kubectl determines if it should use in-cluster authentication. Our particular use case we don't require the pod to be able to internally communicate with the K8s API so this seemed like a slightly more flexible approach. I'm not opposed to also including that as part of the check though.

kleinschmidt commented 1 year ago

Okay, I think that's fine. These ENV variables aren't something a user would ever have set outside a cluster right?

omus commented 1 year ago

Okay, I think that's fine. These ENV variables aren't something a user would ever have set outside a cluster right?

No, I wouldn't expect these to be used outside of the context of a running pod