-
Notifications
You must be signed in to change notification settings - Fork 167
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
refactor(helmraiser): Abstract template code #363
Conversation
Abstracts the code that calls `helm template` into a struct method, so it can be used beyond the Jsonnet interface. This enables better testing, is more readable and also leverages more of the existing code in this project.
var buf bytes.Buffer | ||
cmd.Stdout = &buf | ||
cmd.Stderr = os.Stderr | ||
|
||
// snake_case string | ||
normalizeName := func(s string) string { | ||
s = strings.ReplaceAll(s, "-", "_") | ||
s = strings.ReplaceAll(s, ":", "_") | ||
s = strings.ToLower(s) | ||
return s | ||
} | ||
if err := cmd.Run(); err != nil { | ||
return nil, errors.Wrap(err, "Expanding Helm Chart") | ||
} |
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.
This could all be wrapped in the Helm.run() function, I would even think of returning func (c *Cmd) Output() ([]byte, error)
for this.
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.
(bit awkward that github diff included the normalizeName, just ignore that)
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.
Reusing the same pattern that proved working well in pkg/kubernetes/client
:
tanka/pkg/kubernetes/client/exec.go
Line 21 in 3ff6c70
func (k Kubectl) ctl(action string, args ...string) *exec.Cmd { |
Returning the raw exec.Cmd
allows consuming functions to also modify the commands environment, stdin, capture stderr if required, etc.
I think we will need this in the future here.
var list manifest.List | ||
d := yaml.NewDecoder(&buf) | ||
for { | ||
var m manifest.Manifest | ||
if err := d.Decode(&m); err != nil { | ||
if err == io.EOF { | ||
break | ||
} | ||
return nil, errors.Wrap(err, "Parsing Helm output") | ||
} | ||
list = append(list, m) | ||
} | ||
return files, nil | ||
|
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 this replaces parseYamlToMap
, could you move it into a function and add a unit test for it?
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.
imo the implementation of yaml unmarshalling doesn't require testing here, because go-yaml is heavily unit tested itself, and unmarshalling yaml output into a map[string]interface{}
pretty standard Go stuff.
What I instead would like to test here is that the output of helm
correctly ends up in Jsonnet, regardless of our actual Go implementation.
I will add such a test in a subsequent pull request, which this one is the preparation for.
Abstracts the code that calls
helm template
into a struct method, soit can be used beyond the Jsonnet interface.
This enables better testing, is more readable and also leverages more of
the existing code in this project.
This is extracted from #355