Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/kube improvements #264

Closed

Conversation

olegTarassov
Copy link
Contributor

  1. Test KUBECONFIG global variable instead of file
  • location of kube/config can be moved or appended using several files
  1. Avoid printing kubernetes cluster version unless needed
  • keep terminal line to the minimal
  1. Put some nice colors
  • control context or namespace color
  1. Add Separator for geometry_kube

- Control display of k8s version
- Control color of namespace / context
1) Test KUBECONFIG global variable instead of file
- location of kube/config can be moved or appended using several files
2) Avoid printing kubernetes cluster version unless needed
- keep terminal line to the minimal
3) Put some nice colors
- control context or namespace color
4) Add Separator for geometry_kube
local context="$(kubectl config current-context 2> /dev/null)"
local namespace="$(kubectl config view -o "jsonpath={.contexts[?(@.name==\"$context\")].context.namespace}" 2> /dev/null)"
local geometry_context="$(kubectl config current-context 2> /dev/null)"
local geometry_namespace="$(kubectl config view -o "jsonpath={.contexts[?(@.name==\"$context\")].context.namespace}" 2> /dev/null)"
Copy link
Member

Choose a reason for hiding this comment

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

maybe kube_context and kube_namespace?

Copy link
Member

Choose a reason for hiding this comment

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

Though I can see an argument for this being geometry specific, not kube_specific. Either way, these variables should be 'hidden' unless something breaks, in which case letting the person know 'this is not kube broken, its your prompt' is less panic-inducing.

geometry_context="$(ansi ${GEOMETRY_KUBE_CONTEXT_COLOR:=white} ${geometry_context})"
fi

if [[ ! -z "${GEOMETRY_KUBE_NAMESPACE_COLOR}" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

does this make ${GEOMETRY_KUBE_NAMESPACE_COLOR:=white} redundant?

I think this entire test and else branch can be removed and have the same effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, the default white is redundant.

Secondly, technically we can remove the if else and just have:

geometry_namespace="$(ansi ${GEOMETRY_KUBE_NAMESPACE_COLOR:=white} ${geometry_namespace:=default})"

My reason for the "if / else" is because your terminal foreground colour might not be white like an off-white or grey and the default white would overwrite this unwillingly.

This ways if you dont add GEOMETRY_KUBE_NAMESPACE_COLOR or GEOMETRY_KUBE_CONTEXT_COLOR, you can keep terminal profile colour settings.

I guess this would be the proposed:

  if [[ ! -z "${GEOMETRY_KUBE_CONTEXT_COLOR}" ]]; then
    geometry_context="$(ansi ${GEOMETRY_KUBE_CONTEXT_COLOR} ${geometry_context})"
  fi

  if [[ ! -z "${GEOMETRY_KUBE_NAMESPACE_COLOR}" ]]; then
    geometry_namespace="$(ansi ${GEOMETRY_KUBE_NAMESPACE_COLOR} ${geometry_namespace:=default})"
  fi

# [...]

echo -n "${geometry_kube_separator}${geometry_kube} ${geometry_context}:${geometry_namespace:=default}${geometry_kube_separator}"

Copy link
Member

Choose a reason for hiding this comment

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

I'd also be open to switching ansi's argument order, and making the second argument optional. This would let us possibly remove all of the 'white' defaults easier.

Copy link
Member

Choose a reason for hiding this comment

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

though that kinda messes with the ability to have text with spaces. Which ansi breaks right now too I guess?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, white can be changed to default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes ansi will break if empty.
the geometry_namespace needs the default because kubernetes namespace returns empty string sometimes when namespace is default. This behaviour is not seen observed on the context.

- [`geometry_hg`](#geometryhg)
- [`geometry_hostname`](#geometryhostname)
- [`geometry_jobs`](#geometryjobs)
- [`geometry_kube`](#geometrykube)
- [`geometry_kube`](#geometry_kube)
Copy link
Member

Choose a reason for hiding this comment

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

nice catch!

geometry_namespace="${geometry_namespace:=default}"
fi

echo -n "${geometry_kube_separator}${geometry_kube} ${geometry_context}:${geometry_namespace}${geometry_kube_separator}"
Copy link
Member

Choose a reason for hiding this comment

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

So whats neat is that zsh supports joins that are a bit smarter - you can look at geometry::wrap for how it works, but if you have an array of things you want to join with a string, you can do something like (untested)

geometry_kube_info=($geometry_kube ${geometry_context}:${geometry_namespace})
echo "${(psj.${geometry_kube_separator}.)geometry_kube_info}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me try this out

- Group Variable declaration
- Append kube info to array
- Added ZSH parameter expansion
@olegTarassov
Copy link
Contributor Author

Just Pushed the latest changes that are based on your recommendation.
I have one thing that I dont like or know how to fix in single line:

  kube_namespace="$(kubectl config view -o "jsonpath={.contexts[?(@.name==\"${kube_context}\")].context.namespace}" 2> /dev/null)"
  kube_namespace=${kube_namespace:=default}

I believe there might be a way to overwrite if empty to default

@jedahan
Copy link
Member

jedahan commented Aug 29, 2019

Thanks for the updates, I rebased and merged in #266

@jedahan jedahan closed this Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants