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

WIP: Introduce labels to stevedore context #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jskswamy
Copy link
Collaborator

@jskswamy jskswamy commented Nov 2, 2020

Currently stevedore only supports predefined labels type, environmentType and environment, some of the labels like, environment, environmentType causes confusion to the consumer.

To avoid these confusion, this PR which fixes #1 as well proposes a changes which removes these predefined labels and add them as flexible labels which user themself can define when adding stevedore context, rest of the matches everything works the same way.

current

contexts:
- name: minikube
  kubernetesContext: minikube
  kubeConfigFile: ""
  type: local
  environment: local
  environmentType: dev
current: minikube

proposed

contexts:
- name: minikube
  kubernetesContext: minikube
  kubeConfigFile: ""
  labels:
    type: local
    environment: local
    environmentType: dev
current: minikube

The only feature which this feature couldn't match with the original implementation is the predefined weight assigned to the labels since label is a map[string]string, each and every time the label is read, order could be very different and result in non consistent value getting applied.

To avoid this I'm thinking of sorting the labels in alphabetical order all the time, if we do this we will have the feature parity with the current way of working.

Let me know your thoughts before making such changes.

@jskswamy jskswamy added the enhancement New feature or request label Nov 2, 2020
@jskswamy jskswamy added this to the go-public milestone Nov 2, 2020
@dineshba
Copy link
Collaborator

dineshba commented Nov 2, 2020

Can we have labels section in context file which defines the label's weight?

labels:
- name: context
  weight: 4
- name: environment
  weight: 2
- name: environmentType
  weight: 1

contexts:
- name: staging-1
  kubernetesContext: staging-1
  kubeConfigFile: ""
  labels:
    environment: staging-1
    environmentType: staging
- name: production-1
  kubernetesContext: production-1
  kubeConfigFile: ""
  labels:
    environment: production-1
    environmentType: production
current: staging-1

@jskswamy
Copy link
Collaborator Author

jskswamy commented Nov 3, 2020

What will be the weight if the label is not defined? I can think of two possible solution

  1. Go back to alphabetical order, this could cause more confusion
  2. Ignore the labels which are not defined or automatically update labels section based when the context is added / modified / removed, even in this scenarios what will happen if the user manually removes the entries from the config file then what will be the behavior?

we will be end up adding more code to support this feature. to solve the problem simply why no throw some kind of warning when condition multiple matches

@prabhu43
Copy link
Collaborator

prabhu43 commented Nov 4, 2020

I feel alphabetical sorting of labels does not fulfil the consumer need, it might cause more confusions

I agree with @dineshba. Adding labels section defined in stevedore config helps to avoid confusions.
And also we can make the weight field mandatory for labels.

If we feel this might take lot of effort and time, we can do first release with alphabetical sorting of labels and will create an issue to implement labels section with weights in stevedore config

@prabhu43
Copy link
Collaborator

prabhu43 commented Nov 4, 2020

Another thought: environment should be mandatory in stevedore context, it should not be optional in labels

If there is no different env, no need of going for stevedore. helm is enough

@dineshba
Copy link
Collaborator

dineshba commented Nov 4, 2020

Stevedore prefers configuration over conventions upto some extend. So I feel alphabetical sorting of labels is more of convention and I would like to defer from it.

So first version: Can we go ahead with just three dimensions ?
context, environment, environmentType

Logically, weight will be environmentType < environment < context

Also, I assume code change will be minimal.

@jskswamy jskswamy force-pushed the label-context branch 4 times, most recently from 8c98601 to eb38c31 Compare November 16, 2020 10:04
@arunvelsriram
Copy link
Collaborator

Agree with @dineshba on going ahead with only three dimensions initially.

Also for flexible dimensions, how about just specifying the list of labels and weight will be assigned using our existing implementation:

defaultConditionWeights = NewWeights(knownCriteria)

values[cr] = int(math.Pow(2, float64(index)))

Example:

labels:
  - environment # weight is 2^0 = 1
  - context # weight is 2^1 = 2

contexts:
- name: minikube
  kubernetesContext: minikube
  kubeConfigFile: ""
  labels:
    environment: dev
    context: minikube
current: minikube

@jskswamy
Copy link
Collaborator Author

jskswamy commented Mar 17, 2021

Looks good to me @arunvelsriram @dineshba, will go ahead and implement it

@dineshba
Copy link
Collaborator

I have rebased with latest master @jskswamy ... I can pick up some tasks if there is some in this PR

remove the type, environmentType, environment from context and make
it flexible to specify whatever label consumer wants to specify inorder
to control manifest, override and envs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove type and environment type in Stevedore Context
4 participants