WIP: labelling resources #250
Conversation
pkg/spec/spec.go
Outdated
| // set the name field | ||
| config.Metadata.Name = name | ||
| // set the name field if not given in spec | ||
| if config.Metadata.Name == "" { |
There was a problem hiding this comment.
I was unsure about this change. The spec files have a name in them, but it was being overwritten with the directory path which is an invalid label. I need to understand the context here better on why we were overwriting it in the first place.
There was a problem hiding this comment.
Don't do it this way. The filepath is Tanka's source of truth. Instead, just sanitize the filepath, by substituting all / with -, as my original PR did:
tanka/pkg/spec/v1alpha1/config.go
Lines 38 to 40 in 2f61573
The fact we have names in the spec.json is just a bad side-effect from json marshalling, because we need the field to be included when marshalling for passing to Jsonnet, we can't omitempty or - it.
There was a problem hiding this comment.
ok, makes sense... sort of. I guess it does root the file system at the ksonnet folder somehow, so that helps it be deterministic I suppose. tanka.dev/environment: environments.dns.eu-west2.kube-system is a little bit of a verbose env name, but that's ok.
pkg/spec/spec.go
Outdated
| // set the name field | ||
| config.Metadata.Name = name | ||
| // set the name field if not given in spec | ||
| if config.Metadata.Name == "" { |
There was a problem hiding this comment.
Don't do it this way. The filepath is Tanka's source of truth. Instead, just sanitize the filepath, by substituting all / with -, as my original PR did:
tanka/pkg/spec/v1alpha1/config.go
Lines 38 to 40 in 2f61573
The fact we have names in the spec.json is just a bad side-effect from json marshalling, because we need the field to be included when marshalling for passing to Jsonnet, we can't omitempty or - it.
pkg/tanka/parse.go
Outdated
| for _, manifest := range state { | ||
| labels := manifest.Metadata().Labels() | ||
| labels["app.kubernetes.io/managed-by"] = "tanka" | ||
| labels["tanka.dev/environment"] = env.Metadata.Name |
There was a problem hiding this comment.
These should be constants:
const (
LabelPrefix = "tanka.dev"
LabelEnvironment = LabelPrefix+"/environment"
)
cmd/tk/workflow.go
Outdated
| } | ||
|
|
||
| func workflowFlags(fs *pflag.FlagSet) *workflowFlagVars { | ||
| v := workflowFlagVars{} | ||
| fs.StringSliceVarP(&v.targets, "target", "t", nil, "only use the specified objects (Format: <type>/<name>)") | ||
| v.applyLabels = fs.BoolP("label", "l", false, "Apply tanka metadata labels to all resources") |
There was a problem hiding this comment.
having this as a feature flag sounds dangerous – people will forget to pass it, resulting in unset labels for subsets of resources. I see two options here:
- Just do it. While it will cause changes for every environment, these are purely informative.
- Feature-flag it in
spec.json. See my original pruning PR for that:tanka/pkg/spec/v1alpha1/config.go
Line 47 in 2f61573
There was a problem hiding this comment.
I'm gonna go with 1. I did intend to switch from opt-in to opt-out at some point in the future, but honestly this is either something that should be done or it isn't.
pkg/tanka/parse.go
Outdated
| return &ParseResult{ | ||
| Resources: rec, | ||
| Env: env, | ||
| }, nil | ||
| } | ||
|
|
||
| func applyLabels(state manifest.List, env *v1alpha1.Config) { |
There was a problem hiding this comment.
This does not belong here, it should be part of Reconcile() which is meant to transform resources
There was a problem hiding this comment.
Also I'd prefer not to have functions that modify their input variables. Given this is fairly short, just put it into Reconcile
pkg/kubernetes/manifest/manifest.go
Outdated
| @@ -145,6 +145,13 @@ func (m Metadata) Labels() map[string]interface{} { | |||
| return m["labels"].(map[string]interface{}) | |||
| } | |||
|
|
|||
| func (m Metadata) SetLabel(key, value string) { | |||
There was a problem hiding this comment.
You don't need that, m.Labels() already makes sure it returns a valid map[string]interface{}. Again for modifying maps, I prefer to do it using the Go bracket syntax, because it makes clear that you modify the pointer map there.
There was a problem hiding this comment.
m.Labels() does return a new map, but it does not assign it back into m["labels"], so changes don't stick.
At first I just changed Labels() to do m["labels"] = make...., but I didn't want it to create an empty map simply because something wanted to read the collection.
But if we are going toward the "just do it" approach to labelling, that is not a concern.
There was a problem hiding this comment.
Oh, I mixed up code in my head, I changed it in my PR so it creates AND returns if nil. I'd go forward like so
There was a problem hiding this comment.
Basically nil labels is a thing related to the Go type system, but logically equivalent to an empty map. So it's fine to initiate and return one in case
|
Superceded by #251 |
|
See 347c057 for how to fix the unit tests |
This is my attempt at implementing just resource labeling, independently of any pruning proposals.
Implements #249