-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add support for partials in charts.yml #14
Conversation
@@ -33,5 +34,9 @@ func NewCLIOptions(cli map[string]interface{}) (*CLIOptions, error) { | |||
}, | |||
} | |||
|
|||
if cli["--partials-dir"] != nil { | |||
c.PartialTemplatesPath, _ = cli["--partials-dir"].(string) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To stimulate convention over configuration maybe provide a default for the template path?
} else {
// default
c. PartialTemplatesPath = "config/partials"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure, there isn't really a default that I think would fit all use-cases. I still think 99% of the time you don't need partials, since a charts.yml
is small enough to be less than 50 lines.
At Blendle we use config/deploy/partials
, but that's not a universal standard or anything. And since our CI sets the --partials-dir
by default, and locally we use script/deploy
that abstracts this away, I see no upside to providing a default for this flag.
Feel free to convince me otherwise though 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You right that in 99% of the times we don't need partial but having a default dir and checking if that exists won't be any overhead performance wise but you will get an overhead for the 1% cases you do want to use partials. Since then you need to lookup what being used by our CI or you configure another path and if you switch projects then you will have to look at yet another path.
And sure it's not the universal standard but on the other hand there isn't nothing wrong with enforcing standards of our own if it helps us. Remember we first make the tools for ourselves and just make them public available feels much same like a conversation we once had about another open source project. If other people choose to use another path they can still pass along the cli option ;)
And obviously script/deploy abstracts this away but why should we bother if we have a standard path? I would really like to add a default value, I don't see any negative issues with it, perhaps you have some better arguments to convince me then the once you just mentioned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't have any better arguments, because I still think my original argument is good enough 😋
But, I can see your point, and even though I disagree, I am going to make this change before I merge this PR 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in latest commit 👍
chartsconfig/parser.go
Outdated
func stubChart(b []byte, partialPath string) (*hchart.Chart, error) { | ||
tpls := []*hchart.Template{{Data: []byte(b), Name: "charts.yml"}} | ||
|
||
if partialPath != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you provide a template first check the directory exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depending on wether we provide a default, this would need to change yes. But right now, there's no need, since Walk
already returns an error when it doesn't exist, which is what you want, so you know you did something wrong, instead of silently ignoring the provided path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is great! Thanks @JeanMertz
If the default path is used, and it does not exist, no error is returned. If a non-default path exists and it does not exist, an error is returned. If an empty path is used, no error is returned.
charts.yml
epp
dependencyThe rest of this description is the same as the updated README.md
Partial Templates
You can optionally split your
charts.yml
file into multiple chunks, by usingpartial templates. This works almost the same way as Helm's support for these
in charts. See the Helm documentation for more details.
To use these partials, you have to set the
--partials-dir
flag when callingkubecrt
, pass it the path to your partials directory, and then use thosepartials in your
charts.yml
.Example:
charts.yml:
partials/factorio/resources.yml
You can then run this as follows:
And the result is a fully-parsed charts printed to stdout.
Some notes:
define
has to be uniquely named, or risk being overwrittendefine
blocks in a single file