Sort containers in index order

[ryanbrainard]

This is an implementation of the idea I submitted in #2843. While my follow up questions were not answered yet, I took a stab at this with my own preferences how it should work. I'm happy to adjust as needed.

• Went with "Index" for the name this sort order. Used 'IDX as column header to save horizontal space.
• Showing regular containers as the plain index number. Showing init containers prefixed with a ⓘ and then the index number. I did it this way to keep the visuals simple in the regular case, but mark index when it's in the InitContainers array. This also preserves the actual index in the pod spec. Another benefit of using the Unicode characters as prefixes here is that the ⓘ sorts first so it visually represents the run order of the init containers that run first.
• Dropped the INIT column because it is now redundant with the ⓘ marker. This further saves horizontal space.
• Made IDX the default sort order for containers. This ensures that the containers are listed in the order defined and run (in the case of init containers), which will improve the default UX.

Demo

Pod Spec

Given a pod with these init containers and containers:

apiVersion: v1
kind: Pod
metadata:
  name: rainbow
spec:
  initContainers:
    - name: init-container-red
      image: busybox:1.28
      command: ['sh', '-c', "sleep 5"]
    - name: init-container-orange
      image: busybox:1.28
      command: ['sh', '-c', "sleep 5"]
  containers:
    - name: container-yellow
      image: busybox:1.28
      command: ['sh', '-c', "sleep 3600"]
    - name: container-green
      image: busybox:1.28
      command: ['sh', '-c', "sleep 3600"]
    - name: container-blue
      image: busybox:1.28
      command: ['sh', '-c', "sleep 3600"]

k9s Containers View

Here is how it is represented in k9s in the same order it is defined. Note the IDX column on the left with the init container with ⓘ and their index and regular containers with just their index.

[ryanbrainard]

Just pushed up 2af813b and 0e70b11 to resolve the Codebeat issues where makeContainerRes{} and ContainerRes() had too many fields and arguments by moving Container and Status from fields to methods that look up the values with isInit and index instead of passing them in. This did have a ripple effect everywhere container res was used and makes this PR bigger than I hoped, but does improve the safety a bit and makes it easier to expand to return other things in the future since we have the whole pod available.

[derailed]

@ryanbrainard Thank you for this PR Ryan!
I think that are a few issues here.
In summary, I think the actual update is simple if we determine at the call site what kind of container we are dealing with. Altering the model lead you to much more pain than necessary and resorting to lazy eval could account for additional perf implications.

[ryanbrainard]

@derailed Thanks for the review. I can switch this over to being precomputed. I had something more like that originally and caused Codebeat issues, but I can fully precompute it by removing the isInit arg and likely keep within Codebeat's rules.

[ryanbrainard]

@derailed I just pushed up the requested changes. Looks like this now:

A few things to call out:

• I added in Ephemeral Containers to the containers rendering as requested, but I did not do this in other places like in the pod rendering where it shows container counts.
• I also extracted the container statuses because the way it currently works on master is looking up by name, which could fail if two containers of different types have the same names. The way I changed it should be faster and safer.
• Codebeat is complaining again. It's complaining about complexity, but IMHO, the way it is with the nested loops is better than duplicating the logic in three separate loops. I'm not going to change things again until I hear back from you if this is he right direction.
• Note that the order is now E, I, M, which is alphabetical, but isn't really in run order since Ephemeral Containers would usually be last. That's probably fine since they are rare and at least Init Containers are before the main Containers, but wanted to at least call it out.

[derailed]

@ryanbrainard Thank you for the updates Ryan.

[derailed]

I don't think we need to introduce these groups. If we modified makeContainerRes signature to take in a kind (I,M,E) and an index should be sufficient. Then getContainerStatus can just look up statuses based on kind and index.
What do you think?

[ffais]

@derailed It might be useful to add sidecar containers (S) to the type list. This would help solve this issue #2912