Skip to content
This repository was archived by the owner on Apr 24, 2020. It is now read-only.

[Enhancement] Adding the ability to strip cluster name from AWS ARN if using EKS #1237

Open
Thutm wants to merge 7 commits into
base: next
Choose a base branch
from
Open

[Enhancement] Adding the ability to strip cluster name from AWS ARN if using EKS #1237

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c9b6a0b
Adding the ability to strip cluster name from AWS ARN if using EKS
Apr 2, 2019
bd86167
Had the field extract in wrong spot, this is better
Apr 2, 2019
7278e24
Adding strip eks test, and providing mock AWS ARN string
Apr 3, 2019
d566802
Replacing external binaries with string maniuplation.
Apr 3, 2019
9934af4
Updating to more compact string maniuplation that @dritter provided.
Apr 3, 2019
c20e153
Updating to cleaner syntax and adding example string to reference whe…
Apr 3, 2019
9245dd9
Adding terraform segment and tests
Apr 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion segments/kubecontext/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,11 @@ P9K_KUBECONTEXT_BACKGROUND='blue'
### Customize Icon

The main Icon can be changed by setting `P9K_KUBECONTEXT_ICON="my_icon"`. To change the
icon color only, set `P9K_KUBECONTEXT_ICON_COLOR="red"`.
icon color only, set `P9K_KUBECONTEXT_ICON_COLOR="red"`.

### Strip Cluster and Namespace from AWS ARN (If using EKS)

You may extract the clutser name and namespace from the AWS ARN that is supplied when you use an EKS cluster. This is useful when you really just want to see the cluster name and do not want the full ARN.
```
P9K_KUBECONTEXT_STRIPEKS=true
```
5 changes: 5 additions & 0 deletions segments/kubecontext/kubecontext.p9k
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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\/*//')"
Copy link
Member

@dritter dritter Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Thutm Thutm Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dritter dritter Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dritter dritter Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"${${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.

Copy link
Member

@Syphdias Syphdias Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should do the same thing:

Suggested change
cur_ctx="$(echo $cur_ctx | cut -d':' -f 6 | sed 's/cluster\/*//')"
cur_ctx="${${(s.:.)cur_ctx}[6]#cluster/}"

Copy link
Member

@Syphdias Syphdias Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check before use :)

Copy link
Member

@dritter dritter Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or this: "${${cur_ctx##*\:}/cluster\//}. 😄

Copy link
Member

@Syphdias Syphdias Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}

fi

local k8s_final_text=""

if [[ "$cur_ctx" == "$cur_namespace" ]]; then
Expand Down