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.

Sign up for GitHub

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: labelling resources #250

Closed
wants to merge 6 commits into from
Closed

WIP: labelling resources #250

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
069767e
first pass at labelling resources
captncraig Apr 3, 2020
5dfb4cc
also label export
captncraig Apr 3, 2020
3fb525f
work more safely whether or not labels already exist
captncraig Apr 3, 2020
161d57b
use file path env name again
captncraig Apr 3, 2020
fb46880
make NameLabel helper
captncraig Apr 3, 2020
0f23bd2
code review changes
captncraig Apr 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/tk/export.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func exportCmd() *cli.Command {
res, err := tanka.Show(args[0],
tanka.WithExtCode(getExtCode()),
tanka.WithTargets(stringsToRegexps(vars.targets)...),
tanka.WithLabels(*vars.applyLabels),
)
if err != nil {
return err
Expand Down
7 changes: 6 additions & 1 deletion cmd/tk/workflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@ const (
)

type workflowFlagVars struct {
targets []string
targets []string
applyLabels *bool
}

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")
Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. Just do it. While it will cause changes for every environment, these are purely informative.
  2. Feature-flag it in spec.json. See my original pruning PR for that:

    tanka/pkg/spec/v1alpha1/config.go

    Line 47 in 2f61573

    InjectLabels InjectLabels `json:"injectLabels,omitempty"`

Copy link
Contributor Author

@captncraig captncraig Apr 3, 2020 •
edited by sh0rez
Loading

Choose a reason for hiding this comment

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

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.

return &v
}

Expand All @@ -51,6 +53,7 @@ func applyCmd() *cli.Command {
err := tanka.Apply(args[0],
tanka.WithTargets(stringsToRegexps(vars.targets)...),
tanka.WithExtCode(getExtCode()),
tanka.WithLabels(*vars.applyLabels),
tanka.WithApplyForce(*force),
tanka.WithApplyValidate(*validate),
tanka.WithApplyAutoApprove(*autoApprove),
Expand Down Expand Up @@ -86,6 +89,7 @@ func diffCmd() *cli.Command {
changes, err := tanka.Diff(args[0],
tanka.WithTargets(stringsToRegexps(vars.targets)...),
tanka.WithExtCode(getExtCode()),
tanka.WithLabels(*vars.applyLabels),
tanka.WithDiffStrategy(*diffStrategy),
tanka.WithDiffSummarize(*summarize),
)
Expand Down Expand Up @@ -132,6 +136,7 @@ Otherwise run tk show --dangerous-allow-redirect to bypass this check.`)
pretty, err := tanka.Show(args[0],
tanka.WithExtCode(getExtCode()),
tanka.WithTargets(stringsToRegexps(vars.targets)...),
tanka.WithLabels(*vars.applyLabels),
)
if err != nil {
return err
Expand Down
7 changes: 7 additions & 0 deletions pkg/kubernetes/manifest/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,13 @@ func (m Metadata) Labels() map[string]interface{} {
return m["labels"].(map[string]interface{})
}

func (m Metadata) SetLabel(key, value string) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

if !m.HasLabels() {
m["labels"] = make(map[string]interface{})
}
m["labels"].(map[string]interface{})[key] = value
}

// HasAnnotations returns whether the manifest has annotations
func (m Metadata) HasAnnotations() bool {
return m2o(m).Get("annotations").IsMSI()
Expand Down
6 changes: 4 additions & 2 deletions pkg/spec/spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,10 @@ func Parse(data []byte, name string) (*v1alpha1.Config, error) {
return nil, errors.Wrap(err, "parsing spec.json")
}

// set the name field
config.Metadata.Name = name
// set the name field if not given in spec
if config.Metadata.Name == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

func (m Metadata) NameLabel() string {
return strings.Replace(m.Name, "/", ".", -1)
}

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

config.Metadata.Name = name
}

if err := handleDeprecated(config, data); err != nil {
return config, err
Expand Down
12 changes: 12 additions & 0 deletions pkg/tanka/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,24 @@ func parse(dir string, opts *options) (*ParseResult, error) {
return nil, errors.Wrap(err, "reconciling")
}

if opts.applyLabels {
applyLabels(rec, env)
}

return &ParseResult{
Resources: rec,
Env: env,
}, nil
}

func applyLabels(state manifest.List, env *v1alpha1.Config) {
Copy link
Member

Choose a reason for hiding this comment

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

This does not belong here, it should be part of Reconcile() which is meant to transform resources

Copy link
Member

Choose a reason for hiding this comment

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

Also I'd prefer not to have functions that modify their input variables. Given this is fairly short, just put it into Reconcile

for _, manifest := range state {
meta := manifest.Metadata()
meta.SetLabel("app.kubernetes.io/managed-by", "tanka")
meta.SetLabel("tanka.dev/environment", env.Metadata.Name)
}
}

// eval returns the raw evaluated Jsonnet and the parsed env used for evaluation
func eval(dir string, extCode map[string]string) (raw map[string]interface{}, env *v1alpha1.Config, err error) {
baseDir, env, err := loadDir(dir)
Expand Down
10 changes: 10 additions & 0 deletions pkg/tanka/tanka.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ type options struct {
// target regular expressions to limit the working set
targets []*regexp.Regexp

// whether to add tanka metadata labels
applyLabels bool

// additional options for diff
diff kubernetes.DiffOpts
// additional options for apply
Expand Down Expand Up @@ -90,3 +93,10 @@ func WithApplyAutoApprove(b bool) Modifier {
opts.apply.AutoApprove = b
}
}

// WithLabels allows control over whether tanka adds labels to resources or not
func WithLabels(b bool) Modifier {
return func(opts *options) {
opts.applyLabels = b
}
}