[Enhancement] Adding the ability to strip cluster name from AWS ARN if using EKS #1237
Conversation
|
For testing, I need to add a mock AWS ARN string to be returned on by kubectl. Should I add to the existing tests or just duplicate all the current tests referencing "minikube"? I can't think of any reasons for additional mock strings to test against and didn't want to unnecessarily update all existing tests. |
|
Hi @Thutm , Thanks for the PR. I am a fan of one test per use-case, so please add some new tests. I think it will be enough to add a positive and a negative test. |
segments/kubecontext/kubecontext.p9k
Outdated
| @@ -36,6 +36,11 @@ prompt_kubecontext() { | |||
| cur_namespace="default" | |||
| fi | |||
|
|
|||
| # Extract cluster name and namespace from AWS ARN, if enabled. | |||
| if [[ "$P9K_KUBECONTEXT_STRIPEKS" == true ]]; then | |||
| cur_ctx="$(echo $cur_ctx | cut -d':' -f 6 | sed 's/cluster\/*//')" | |||
There was a problem hiding this comment.
I am pretty sure that can be done without piping to external programs. Once you added some tests and I know what $cur_ctx is, I can help you with that.
There was a problem hiding this comment.
Sure, i'll add new tests rather than modifying. The $cur_ctx string that comes back when using AWS EKS is something like this:
arn:aws:eks:us-east-1:1234567890:cluster/dev-eks/default
Where 1234567890 is the account ID. It follows the AWS ARN pattern. I put screenshots in the PR description. Do we typically want to avoid using external binaries to retrieve the output we want? I noticed some other segments were using cut/sed so I was trying to follow current practice.
There was a problem hiding this comment.
Do we typically want to avoid using external binaries to retrieve the output we want? I noticed some other segments were using cut/sed so I was trying to follow current practice.
Yes. At least to a certain extend. It is usually vastly slower to execute an external binary, than to query a variable. But, on the other hand, if the variable is only valid in certain circumstances, than it might be better to call the external binary. As a rule of thumb, external calls should be avoided as much as possible. Regarding the use of cut/sed: We are currently trying to get rid of all these calls, but we are not finished yet.
There was a problem hiding this comment.
"${${cur_ctx##*\:}/cluster\//}" This strips everything before the last colon away, and removes cluster/. So this should have the same effect like | cut -d':' -f 6 | sed 's/cluster\/*//', without the subshells.
segments/kubecontext/kubecontext.p9k
Outdated
| @@ -36,6 +36,11 @@ prompt_kubecontext() { | |||
| cur_namespace="default" | |||
| fi | |||
|
|
|||
| # Extract cluster name and namespace from AWS ARN, if enabled. | |||
| if [[ "$P9K_KUBECONTEXT_STRIPEKS" == true ]]; then | |||
| cur_ctx="$(echo $cur_ctx | cut -d':' -f 6 | sed 's/cluster\/*//')" | |||
There was a problem hiding this comment.
This should do the same thing:
| cur_ctx="$(echo $cur_ctx | cut -d':' -f 6 | sed 's/cluster\/*//')" | |
| cur_ctx="${${(s.:.)cur_ctx}[6]#cluster/}" |
There was a problem hiding this comment.
Or this: "${${cur_ctx##*\:}/cluster\//}. 😄
There was a problem hiding this comment.
I hope there is no : in the last part. Because then all suggestions fail 😆
If there are not additional /s then you could even go for ${cur_ctx:t}
|
Thanks all, I actually ended up using: and then updated it to dritters suggestion. Let me know if that works. |
…n working on the pattern matching
|
Attached segment screenshot. No terraform specific icon that I could find. Used a globe instead for now. |
This is an enhancement to the existing kubecontext segment. While using AWS EKS kubeconfig's this will strip the cluster name out of the ARN to stop bloating your command line.
Before:
After:
