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

feat: Advanced Templating #823

Merged
merged 20 commits into from
Aug 31, 2019
Merged

Conversation

astorath
Copy link
Contributor

@astorath astorath commented Aug 26, 2019

  1. Added helmfile build command to print final state
    Motivation: useful for debugging purposes and some CI scenarios
  2. Template interpolation is now recursive (you can cross-reference release fields) like:
templates:
  release:
    name: {{`app-{{ .Release.Namespace }}`}}
    namespace: {{`{{ .Release.Labels.ns }}`}}
    labels:
      ns: dev
  1. Experimental: Added some boolean release fields interpolation in templates:
templates:
  release:
    name: {{`app-{{ .Release.Namespace }}`}}
    namespace: dev
    installedTemplate: {{`{{ eq .Release.Namespace "dev" }}`}}

this solves: #818

  1. Added more template interpolations: Labels, SetValues
  2. Added template interpolation for inline Values
  3. Added helmfile list command to print target releases in simple tabular form
  4. Added release names in some helm output messages, e.g.: Comparing release=%v, chart=%v

TODO:

  • add some docs
  • add values (full) templating
  • make some yaml serialization improvements

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 28, 2019

@astorath Hey! Thanks a lot for your efforts.

I'm looking forward to review this fully once this gets out of the WIP status.

Until then I'll keep adding design comments. Firstly:

Template interpolation is now recursive (you can cross-reference release fields) like:

How the dependencies across field templates are handled? Do you by any chance build something like a DAG of field templates?

Experimental: Added some boolean release fields interpolation in templates:

This makes sense. An alternative way to that is to give boolean field interface{} types and render it when and only when the actual value type was string. Have you considered it? If so, would u mind sharing why you prefer doing it with another -Template fields?

Added helmfile state command to print final state
Motivation: useful for debugging purposes and some CI scenarios

Awesome!

Can I later enhance it so that the output of the state command can be fed into helmfile apply for reproducible and audited deployments?

Actually I'm talking about #780 so I'd be glad if I can rename it to helmfile build. Thoughts?

@astorath
Copy link
Contributor Author

@mumoshu

How the dependencies across field templates are handled? Do you by any chance build something like a DAG of field templates?

No, just simple iterations with some sane limit. If release stops mutating: interpolation stops.

This makes sense. An alternative way to that is to give boolean field interface{} types and render it when and only when the actual value type was string. Have you considered it? If so, would u mind sharing why you prefer doing it with another -Template fields?

Well, I do consider my approach ugly. But I don't think that messing with HelmSpec (changing some field types) is a good option: all consumers would have to cast those or use some getters on public field...

As I've pointed out, the best way (IMHO) is to separate HelmSpec and HelmTemplateSpec. Maybe treat HelmTemplateSpec as map[string]interface{}. But this approach requires a lot of effort and my humble golang knowledge is not enough for such an endeavour...

Nevertheless, if you think using interface{} for those fields with some getter methods is a better option - this is definitely an option.

Actually I'm talking about #780 so I'd be glad if I can rename it to helmfile build. Thoughts?

I've replaced the name.

@astorath astorath marked this pull request as ready for review August 30, 2019 23:35

func ptr(v interface{}) interface{} {
r := v
return reflect.ValueOf(r).Addr().Interface()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks neat!

@mumoshu mumoshu changed the title WIP: Advanced Templating Advanced Templating Aug 31, 2019
Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

helmfile list and helmfile build with support for templating non-string release fields! LGTM. Thanks a lot for your AWESOME work @astorath 🎉

@mumoshu mumoshu changed the title Advanced Templating feat: Advanced Templating Aug 31, 2019
@mumoshu
Copy link
Collaborator

mumoshu commented Aug 31, 2019

No, just simple iterations with some sane limit. If release stops mutating: interpolation stops.

Understood. As long as someone tries 6+ iterations in a real world project(not sure someone ever does!), we can live with that, which makes me think that it's a great starting point.

the best way (IMHO) is to separate HelmSpec and HelmTemplateSpec

I agree a lot!

Maybe treat HelmTemplateSpec as map[string]interface{}

This may negate my previous comment but I started thinking that both map[string]interface{} and overloading makes it harder for users to enable/disable templating on the specific field(name, namespace, tillerless and so on there will be no prob. I'm curious about values and set)

The way you've done things with the -Template suffix would never suffer from that, which is good.

Probably I'll adapt existing fields like values and set to be more specific about templating enablement in the future.

- `set` block values:
```yaml
# ...
set:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accordingly to #823 (comment), I'd be happy if we could enable templates only for setTemplate and valuesTemplate rather than set and values respectively.

I'll make the changes on my own before releasing this, but if you made it on your side I'd be happy to review/accept the PR! Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #833

Copy link
Collaborator

Choose a reason for hiding this comment

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

Merged #833. Please feel free to leave any comments about it!

@mumoshu mumoshu merged commit 11d0abb into roboll:master Aug 31, 2019
mumoshu added a commit that referenced this pull request Aug 31, 2019
mumoshu added a commit that referenced this pull request Aug 31, 2019
mumoshu added a commit that referenced this pull request Aug 31, 2019
mumoshu added a commit that referenced this pull request Aug 31, 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.

2 participants