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 for kubecontext section: Custom colors per context/namespace #576

Merged
merged 10 commits into from Dec 11, 2018
Merged

Feature for kubecontext section: Custom colors per context/namespace #576

merged 10 commits into from Dec 11, 2018

Conversation

nomaed
Copy link
Contributor

@nomaed nomaed commented Nov 30, 2018

Description

This change adds a new feature for the kubecontext section.
It allows setting SPACESHIP_KUBECONTEXT_COLORGROUPS array with colors and patterns that match context names and namespaces, that should change the section to the specified color. This is usually going to be critical environments that we need to make sure we don't modify by mistake, such as "production" or "staging" (as opposed to regular "dev").

This is an idea by a colleague after too many people not noticing that they are on the production environment and deploying changes, when all they wanted is to test changes in local development env.

Screenshot

screen shot 2018-12-01 at 19 17 42

@maximbaz
Copy link
Contributor

This is interesting idea, I'd totally use it myself.

Here's what I'd like to be added:

  1. Support more than one group with different colors (most common groups would be "staging" and "production", but for example we also have an extra cluster for testers and an extra cluster for various early experiments — in my case I would like to avoid experimenting on "dev" cluster).
  2. Support regex rule for defining groups, speaking only of production clusters we have 8 of them and I would be annoyed to list them all. But they all follow a naming scheme, so I'd really like to define rules as *-prod-* for production, *-dev-* for development, etc.

I'm wondering how to make the first point generic enough, I don't think hardcoding "popular" groups is good enough. Ideally we allow people to configure as many "rulepattern-color" pairs as they want.

@maximbaz
Copy link
Contributor

Just realized that the second point will actually work out of the box, if you don't wrap your name in quotes.

image

So we only need to change how we define pattern-color pairs, to make it more generic.

@nomaed
Copy link
Contributor Author

nomaed commented Nov 30, 2018

Regex

I love the idea of regex, that will definitely make life easier.
However, not sure that your example will work since you can't define such patterns in a list AFAIK:

$ arr=(*-dev-*)
zsh: no matches found: *-dev-*

It can work with actual regex with =~ comparator, although this means that any regex-special character in the name will need to be escaped and it will be less user friendly.

ctx=(".*-dev-.*" ".*-prod$" exact-context)
...
if [[ $kube_context =~ $crit ]]; then ...

Do you think it's good enough to specify that the list of context names is a list of regular expressions rather than strings or expandable patterns? Or is there another way to achieve adding these patterns to a list, to use the syntax you suggested?

Edit:
This doesn't work...

$ setopt GLOB_SUBST; [[ "my-dev-cl" == "*-dev-*" ]] && echo y || echo n; unsetopt GLOB_SUBST
n

Groups

I agree that it's a great idea, but it sounds like a lot of pain for the user to configure.
Initially I thought that an assoc array can make it quite simple, something like bash syntax:

ctx=(
  [red] = (*-prod *-staging)
  [yellow] = (*-dev-*)
)

but discovered very fast that it's not a supported syntax, and that having a multi-dimensional array is not supported either and is much more pain to implement than its worth :)

The next idea is something like:

USE_GROUPS=(prod dev blah)
GROUP_prod_COLOR=red
GROUP_prod_PATTERNS=(".+-prod" ".+-staging")
GROUP_dev_COLOR=yellow
GROUP_dev_PATTERNS=(".+-dev-.+")

or something in that direction. However, the list of group "names" is really not useful, it's just there to be able to build a list of variables to iterate over (and I still haven't figured how to use regex comparison...)

@maximbaz
Copy link
Contributor

maximbaz commented Nov 30, 2018

Agree on using regex, and using associative arrays is cool too.

How about this approach?

declare -A SPACESHIP_KUBECONTEXT_COLORGROUPS=(
  [-test-]=yellow
  [-prod-]=red
  [-staging-]=red
)

cluster="my-prod-cluster"

for pattern color in ${(kv)SPACESHIP_KUBECONTEXT_COLORGROUPS}; do
  echo "testing $cluster against $pattern"
  if [[ "$cluster" =~ "$pattern" ]]; then
    echo "matches $pattern!!!"
  fi
done

UPDATE: apparently .*-test-.* is the same as -test-, so simplified.

@nomaed
Copy link
Contributor Author

nomaed commented Dec 1, 2018

@maximbaz Your syntax is very usable :)

I went with your suggestion and changed the code to support SPACESHIP_KUBECONTEXT_COLORGROUPS and it works quite well!

typeset -A SPACESHIP_KUBECONTEXT_COLORGROUPS=(
  ['dev-system']=green
  ['production']=red
  ['staging']=red
  ['^test-[0-9]+$']=yellow
  ['\.k8s\.local$']=red
)

@maximbaz
Copy link
Contributor

maximbaz commented Dec 1, 2018

Now it's very cool 👍 Can you explain why you have == || =~ in code, why not just =~ alone?

@nomaed
Copy link
Contributor Author

nomaed commented Dec 1, 2018

I figured that testing for equality first might have a tiny benefit for performance. If there's exact match, there's really no need to do a regex match.
However, I haven't tested whether it has any actual performance benefit and it might be just a case premature optimization on my side...

Also, the idea that you can set up first dev as exact match and then dev-.+ as a different color. If there is only =~, having "dev" will also match "dev-123" so you'd have to specify it as ^dev$

@maximbaz
Copy link
Contributor

maximbaz commented Dec 1, 2018

I wouldn't worry about this, use =~ alone and once spaceship is officially async it wouldn't matter at all.

sections/kubecontext.zsh Outdated Show resolved Hide resolved
sections/kubecontext.zsh Outdated Show resolved Hide resolved
sections/kubecontext.zsh Outdated Show resolved Hide resolved
sections/kubecontext.zsh Outdated Show resolved Hide resolved
docs/Options.md Outdated Show resolved Hide resolved
sections/kubecontext.zsh Outdated Show resolved Hide resolved
Copy link
Contributor

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

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

Works beautifully, nice addition

@nomaed nomaed changed the title Feature for kubecontext section: "Critical" context names Feature for kubecontext section: Custom color per context/namespace Dec 1, 2018
@nomaed nomaed changed the title Feature for kubecontext section: Custom color per context/namespace Feature for kubecontext section: Custom colors per context/namespace Dec 1, 2018
@nomaed
Copy link
Contributor Author

nomaed commented Dec 1, 2018

Thanks for the reviews!

@maximbaz
Copy link
Contributor

maximbaz commented Dec 8, 2018

Friendly ping @salmanulfarzy, this is something very cool and prevents accidentally messing with production environment, definitely worth merging and letting people know that this functionality exists.

Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

This looks great @nomaed 👏 Proposing some minor changes.

docs/Options.md Outdated Show resolved Hide resolved
sections/kubecontext.zsh Outdated Show resolved Hide resolved
sections/kubecontext.zsh Outdated Show resolved Hide resolved
docs/Options.md Outdated Show resolved Hide resolved
@salmanulfarzy salmanulfarzy added improvement A PR that make small changes for improving UX, performance, readability, etc review-required labels Dec 9, 2018
nomaed and others added 7 commits December 11, 2018 09:15
Feature: add the ability to specify section color based on context names
matching a pattern.

Main use case is to mark contexts that should be handled with care, such
as "staging" or "production" environments.

An example of a use case might be to add the following to .zshrc:
```zsh
typeset -A SPACESHIP_KUBECONTEXT_COLORGROUPS=(
  ['dev-system']=green
  ['production']=red
  ['staging']=red
  ['^test-[0-9]+$']=yellow
  ['\.k8s\.local$']=red
)
```
Co-Authored-By: nomaed <nomaed@users.noreply.github.com>
@nomaed
Copy link
Contributor Author

nomaed commented Dec 11, 2018

I made the changes per review comments:

  • Options.md specifies the default value to be empty
  • Options.md example: comments in code are one line above to make it cleaner
  • Renamed SPACESHIP_KUBECONTEXT_COLORGROUPS to SPACESHIP_KUBECONTEXT_COLOR_GROUPS
  • Quotes unassigned local variable definitions

Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Thank you @nomaed, Made some minor changes myself.

docs/Options.md Outdated Show resolved Hide resolved
docs/Options.md Show resolved Hide resolved
Co-Authored-By: nomaed <nomaed@users.noreply.github.com>
Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Code looks great @nomaed

@nomaed
Copy link
Contributor Author

nomaed commented Dec 11, 2018

Super, thank you guys!

@salmanulfarzy salmanulfarzy merged commit 4a7aaf8 into spaceship-prompt:master Dec 11, 2018
@salmanulfarzy
Copy link
Member

Thank you for contributing 🎉

@nomaed @maximbaz can you guys share screenshot/gif to be shared on Twitter with default settings ? Do share your twitter handle if you've one 😄

dedene pushed a commit to zenjoy/spaceship-prompt that referenced this pull request Feb 24, 2019
It allows setting SPACESHIP_KUBECONTEXT_COLOR_GROUPS array with colors 
and patterns that match context names and namespaces, that should change
the section to the specified color. 

This is usually going to be critical environments that we need to make 
sure we don't modify by mistake, such as "production" or "staging" 
(as opposed to regular "dev").

Co-Authored-By: Maxim Baz <maximbaz@users.noreply.github.com>
zachliu pushed a commit to zachliu/spaceship-prompt that referenced this pull request Oct 25, 2019
It allows setting SPACESHIP_KUBECONTEXT_COLOR_GROUPS array with colors 
and patterns that match context names and namespaces, that should change
the section to the specified color. 

This is usually going to be critical environments that we need to make 
sure we don't modify by mistake, such as "production" or "staging" 
(as opposed to regular "dev").

Co-Authored-By: Maxim Baz <maximbaz@users.noreply.github.com>
denysdovhan pushed a commit that referenced this pull request May 27, 2021
It allows setting SPACESHIP_KUBECONTEXT_COLOR_GROUPS array with colors 
and patterns that match context names and namespaces, that should change
the section to the specified color. 

This is usually going to be critical environments that we need to make 
sure we don't modify by mistake, such as "production" or "staging" 
(as opposed to regular "dev").

Co-Authored-By: Maxim Baz <maximbaz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement A PR that make small changes for improving UX, performance, readability, etc
Development

Successfully merging this pull request may close these issues.

None yet

3 participants