-
Notifications
You must be signed in to change notification settings - Fork 321
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
Embed Helm chart templates in CLI #757
Conversation
7daee40
to
53d329e
Compare
cli/cmd/common/utils.go
Outdated
Name: valuesFileName, | ||
Data: bytes, | ||
}, | ||
) |
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 could wrap this behavior in a separate function to reduce code duplication.
func readChart(chartDirName, fileName) (*chartFile, error) {
bytes, err = chart.ReadFile(fmt.Sprintf("%s/%s", chartDirName, fileName))
if err != nil {
return nil, err
}
return &loader.BufferedFile{
Name: fileName,
Data: bytes,
}
}
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.
I took a stab at a fun refactor using Thomas's suggestion and using filepath.Rel
to do the removal of charts/
:
func ReadChartFiles(chart embed.FS, chartDirName string) ([]*loader.BufferedFile, error) {
var chartFiles []*loader.BufferedFile
// Load Chart.yaml and values.yaml first.
for _, f := range []string{chartFileName, valuesFileName} {
buffered, err := readFile(chart, filepath.Join(chartDirName, f), chartDirName)
if err != nil {
return nil, err
}
chartFiles = append(chartFiles, buffered)
}
// Now load everything under templates/.
dirs, err := chart.ReadDir(filepath.Join(chartDirName, templatesDirName))
if err != nil {
return nil, err
}
for _, dir := range dirs {
if dir.IsDir() {
// We only need to package up files.
continue
}
buffered, err := readFile(chart, filepath.Join(chartDirName, templatesDirName, dir.Name()), chartDirName)
if err != nil {
return nil, err
}
chartFiles = append(chartFiles, buffered)
}
return chartFiles, nil
}
func readFile(chart embed.FS, f string, basePath string) (*loader.BufferedFile, error) {
bytes, err := chart.ReadFile(f)
if err != nil {
return nil, err
}
rel, err := filepath.Rel(basePath, f)
if err != nil {
return nil, err
}
return &loader.BufferedFile{
Name: rel,
Data: bytes,
}, 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.
Could definitely be cleaned up but might be a nice change.
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.
Great suggestion @t-eckert and thanks for giving it a shot Luke, it pretty much looks good to me that way. I ended up only changing a few variable names from what it looks like here
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 looks great! I had some stylistic nits, but nothing major. You are doing great with 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.
Nice work!
For future selves: Nitya and I discussed instead of doing this, pinning the chart version that helm downloads using version/version.go
. This would reduce the complexity here but this work has the nice effect that there are no external calls to Helm/repositories and that everything is packaged up in the compiled binary so based on that we decided to continue with this approach.
cli/cmd/common/utils.go
Outdated
Name: valuesFileName, | ||
Data: bytes, | ||
}, | ||
) |
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.
I took a stab at a fun refactor using Thomas's suggestion and using filepath.Rel
to do the removal of charts/
:
func ReadChartFiles(chart embed.FS, chartDirName string) ([]*loader.BufferedFile, error) {
var chartFiles []*loader.BufferedFile
// Load Chart.yaml and values.yaml first.
for _, f := range []string{chartFileName, valuesFileName} {
buffered, err := readFile(chart, filepath.Join(chartDirName, f), chartDirName)
if err != nil {
return nil, err
}
chartFiles = append(chartFiles, buffered)
}
// Now load everything under templates/.
dirs, err := chart.ReadDir(filepath.Join(chartDirName, templatesDirName))
if err != nil {
return nil, err
}
for _, dir := range dirs {
if dir.IsDir() {
// We only need to package up files.
continue
}
buffered, err := readFile(chart, filepath.Join(chartDirName, templatesDirName, dir.Name()), chartDirName)
if err != nil {
return nil, err
}
chartFiles = append(chartFiles, buffered)
}
return chartFiles, nil
}
func readFile(chart embed.FS, f string, basePath string) (*loader.BufferedFile, error) {
bytes, err := chart.ReadFile(f)
if err != nil {
return nil, err
}
rel, err := filepath.Rel(basePath, f)
if err != nil {
return nil, err
}
return &loader.BufferedFile{
Name: rel,
Data: bytes,
}, nil
}
cli/cmd/common/utils.go
Outdated
Name: valuesFileName, | ||
Data: bytes, | ||
}, | ||
) |
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.
Could definitely be cleaned up but might be a nice change.
This will help debug errors in connections between static-client and static-server after federation has been set up. This will provide information about whether DC1 (static-client) is aware of the static-server deployment in DC2.
a7d74b9
to
3b1f762
Compare
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.
🎉 super cool!
passes unit and CLI smoke tests
This reverts commit 3c07a08.
3b1f762
to
71fd91e
Compare
Changes proposed in this PR:
How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: