-
Notifications
You must be signed in to change notification settings - Fork 943
[Enhancement] Adding the ability to strip cluster name from AWS ARN if using EKS #1237
base: next
Are you sure you want to change the base?
Changes from 3 commits
c9b6a0b
bd86167
7278e24
d566802
9934af4
c20e153
9245dd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should do the same thing:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please check before use :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or this: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope there is no |
||||||
fi | ||||||
|
||||||
local k8s_final_text="" | ||||||
|
||||||
if [[ "$cur_ctx" == "$cur_namespace" ]]; then | ||||||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 removescluster/
. So this should have the same effect like| cut -d':' -f 6 | sed 's/cluster\/*//'
, without the subshells.