fix(server): fix find container logic for terminal #26858
fix(server): fix find container logic for terminal #26858linghaoSu wants to merge 1 commit intoargoproj:masterfrom
Conversation
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
linghaoSu
force-pushed
the
fix/init-container-terminal
branch
2 times, most recently
from
856f1dc to
d18f55f
Compare
Signed-off-by: linghaoSu <linghao.su@daocloud.io>
linghaoSu
force-pushed
the
fix/init-container-terminal
branch
from
d18f55f to
208d3c0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26858 +/- ##
=======================================
Coverage 63.06% 63.07%
=======================================
Files 414 414
Lines 56286 56287 +1
=======================================
+ Hits 35499 35502 +3
+ Misses 17418 17417 -1
+ Partials 3369 3368 -1 ☔ View full report in Codecov by Sentry. |
dudinea
left a comment
dudinea
left a comment
There was a problem hiding this comment.
@linghaoSu thank you for noticing and fixing that, great work!
I tried and it worked fine as you described.
I noticed one problem: the web terminal still cannot access a regular init container (without restartPolicy: Always) while it still executing (pod stuck initializing). I get error 400 fromm the web socket in such a case.
I can do kubectl exec into such a container just fine.
Would it possible to fix this as well?
| } | ||
| } | ||
| if !findContainer { | ||
| if !ContainerExists(pod, container) { |
There was a problem hiding this comment.
ContainerExists() is not used from outside this package, probably should start from small letter
| } | ||
|
|
||
| func ContainerExists(pod *corev1.Pod, containerName string) bool { | ||
| allContainers := slices.Concat(pod.Spec.Containers, pod.Spec.InitContainers) |
There was a problem hiding this comment.
slices.Concat() allocates new slice and copies all data from the source containers into it. And the data is the full container specs.
It seems to be wasteful, I'd just check both arrays in sequence instead.
There was a problem hiding this comment.
Maybe, you can use slices.Contains :)
3 participants
fix #26830
Before
terminal-before.mp4
After
terminal-after.mp4
Checklist: