Refactor Helm template file writing logic #385

ebramanti · 2018-08-17T00:13:32Z

What I Did

Fixes #373 by refactoring how we write a rendered Helm asset to its destination.

How I Did it

Create base destination directory before renaming rendered folders
Unify app/init file-writing logic by refactoring to use rootFs
- app command now removes nested helm template chart dir, writes rendered templates in dest root
Fix noconfig.go to properly pass Root parameter
Add additional integration test for Helm charts with sub-charts
Improve messaging and errors around templating

How to verify it

Unit and integration tests were updated to verify that this issue is addressed.

Description for the Changelog

Helm asset rendering now writes base destination directory

Picture of a Boat (not required but encouraged)

- Fix #373 by creating base destination directory - Unify app/init file-writing logic

…t-directory

laverya · 2018-08-18T00:06:23Z

pkg/constants/filepaths.go

@@ -19,7 +19,7 @@ const ReleasePath = ".ship/release.yml"
 const KustomizeHelmPath = "chart"

 // RenderedHelmTempPath is the path where the `helm template` command writes to
-const RenderedHelmTempPath = ".ship/tmp-rendered"
+const RenderedHelmTempPath = "tmp-rendered"


why is this no longer in .ship? I think that's where we're putting all of our temp directories

@laverya Change of plans on this because of rootFs, to be consistent in the filesystem instance we use we are writing them to the Lifecycle Root and then cleaning up after ourselves.

Can you remind me why we did this? Is this an issue with moving things in/out of rootFs?

@dexhorthy We did this because of how BasePathFs works with TempDirs. We would have to use 2 FS' (one for temporary, one for root) to manage this

…t-directory

laverya · 2018-08-18T01:10:33Z

integration/base/helm-fetch/metadata.yaml

@@ -1,3 +1,3 @@
 disable_online: true

-skip_cleanup: true
+skip_cleanup: false


whoops, my bad

laverya · 2018-08-18T01:11:55Z

pkg/lifecycle/render/helm/fetch.go

@@ -87,7 +87,7 @@ func (f *ClientFetcher) FetchChart(
 		}

 		// find the path that the chart was fetched to
-		chartDir, err := util.FindOnlySubdir(checkoutDir, f.FS)
+		chartDir, err := util.FindOnlySubdir(checkoutDir, f.FS.Fs)


holy mother of stutter
(unavoidable though)

laverya · 2018-08-18T01:13:54Z

pkg/lifecycle/render/helm/template.go

+		return path.Join(constants.RenderedHelmTempPath, meta.HelmChartMetadata.Name), nil
+	}
+
+	return util.FindOnlySubdir(constants.RenderedHelmTempPath, rootFs.Fs)


#392 (comment)
lol

dexhorthy · 2018-08-20T14:29:56Z

pkg/lifecycle/render/helm/template.go

+) error {
+	debug := level.Debug(log.With(f.Logger, "method", "cleanUpAndOutputRenderedFiles"))
+
+	subChartsDirName := "charts"


seems like we're using a lot of constants/paths that are pretty ship init <chart> specific

I checked out the changes to integration test expected and they look good though. I like that we're stripping off templates/ everywhere now and just dropping the rendered stuff at Dest

@dexhorthy This is no longer ship init-specific, I unified the logic for helm templating

dexhorthy · 2018-08-20T14:34:41Z

pkg/lifecycle/render/helm/template_test.go

@@ -138,10 +140,15 @@ func TestForkTemplater(t *testing.T) {
 			mockCommands.EXPECT().DependencyUpdate(chartRoot).Return(nil)
 			mockCommands.EXPECT().Template(chartRoot, templateArgs).Return(nil)

+			mockFolderPathToCreate := path.Join(constants.RenderedHelmTempPath, expectedChannelName, "templates")
+			if err := mockFs.MkdirAll(mockFolderPathToCreate, 0755); err != nil {
+				req.Error(err, "failed to make temp dir")


req.Error asserts that an error is present. I think you want req.NoError

You can also skip the if, require will essentially panic and abort the test if the assertion fails

dexhorthy · 2018-08-20T14:36:03Z

pkg/lifecycle/render/root/fs.go

@@ -20,14 +20,17 @@ func (f *Fs) TempDir(prefix, name string) (string, error) {
 }

 // NewRootFS initializes a Fs struct with a base path of root
-func NewRootFS(root string) Fs {
+func NewRootFS(fs afero.Afero, root string) Fs {


dexhorthy · 2018-08-20T14:37:10Z

pkg/specs/chart.go

@@ -396,7 +396,7 @@ func (r *Resolver) fetchUnpack(chartRef, repoURL, version, dest, home string) (s
 		return out, err
 	}

-	subdir, err := util.FindOnlySubdir(tmpDest, r.FS)
+	subdir, err := util.FindOnlySubdir(tmpDest, r.FS.Fs)


it seems a little strange to me that we strip off the fs here, just to make a new Afero with it down below. Why not just pass through the Afero instance?

That's how it was originally, but it seemed weird having to initialize a new afero.Afero in some cases.

I'm going to put it back

dexhorthy

approved but a few small things to tweak

This reverts commit bb62a7a.

This reverts commit 4cc5ccb.

Refactor Helm template file writing logic

16dc720

- Fix #373 by creating base destination directory - Unify app/init file-writing logic

ebramanti requested review from laverya and dexhorthy August 17, 2018 00:13

ebramanti added the wip Work in progress label Aug 17, 2018

ebramanti added 14 commits August 16, 2018 17:16

Merge remote-tracking branch 'upstream/master' into helm-template-des…

5cdda31

…t-directory

Move files around in tests

c48f8f7

WIP filesystem stuff

c59593c

Merge branch 'master' into helm-template-dest-directory

e31d899

Update file structure of helm-fetch test

85edc72

Improve messaging and errors around templating

4dcd307

Merge remote-tracking branch 'upstream/master' into helm-template-des…

10bef99

…t-directory

Set up folder structure for template_test

0e5ae72

Fix noconfig to properly pass Root parameter

bdc2463

Add new integration test for nested Helm charts

62a3844

Move files into correct dir for test

4ea6f09

Update file contents to be written like helm template

9418c7d

Add back sub chart copy

e776a1e

Better naming for test type

1a7d705

ebramanti removed the wip Work in progress label Aug 17, 2018

ebramanti added 3 commits August 17, 2018 14:11

Fix a test 😄

f0e6a19

Pass an Afero into rootFs constructor

3606d7b

Directly use afero instance passed if root is current working directory

9d83e55

laverya reviewed Aug 18, 2018

View reviewed changes

ebramanti added 3 commits August 17, 2018 17:10

Lock helm chart version

909f0e3

Merge remote-tracking branch 'upstream/master' into helm-template-des…

cf54804

…t-directory

Accept an Fs interface as parameter of FindOnlySubdir

d15ae1d

laverya reviewed Aug 18, 2018

View reviewed changes

laverya approved these changes Aug 18, 2018

View reviewed changes

dexhorthy reviewed Aug 20, 2018

View reviewed changes

dexhorthy approved these changes Aug 20, 2018

View reviewed changes

ebramanti added 2 commits August 20, 2018 09:41

Revert FindOnlySubdir changes

e058f1e

Fix test error

0fe442e

ebramanti merged commit bb62a7a into replicatedhq:master Aug 20, 2018

ebramanti deleted the helm-template-dest-directory branch August 20, 2018 17:05

ebramanti mentioned this pull request Aug 20, 2018

Revert "Refactor Helm template file writing logic (#385)" #406

Merged

ebramanti added a commit that referenced this pull request Aug 20, 2018

Revert "Refactor Helm template file writing logic (#385)"

4cc5ccb

This reverts commit bb62a7a.

ebramanti mentioned this pull request Aug 20, 2018

Re-merge #385: Refactor Helm template file writing logic #414

Merged

ebramanti added a commit that referenced this pull request Aug 20, 2018

Restore refactor of Helm template file writing logic (see #385)

1b4f05c

This reverts commit 4cc5ccb.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Helm template file writing logic #385

Refactor Helm template file writing logic #385

ebramanti commented Aug 17, 2018 •

edited

Loading

laverya Aug 18, 2018

ebramanti Aug 18, 2018

dexhorthy Aug 20, 2018

ebramanti Aug 20, 2018

laverya Aug 18, 2018

laverya Aug 18, 2018

laverya Aug 18, 2018

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018

ebramanti Aug 20, 2018

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018

ebramanti Aug 20, 2018

ebramanti Aug 20, 2018 •

edited

Loading

dexhorthy left a comment

Refactor Helm template file writing logic #385

Refactor Helm template file writing logic #385

Conversation

ebramanti commented Aug 17, 2018 • edited Loading

What I Did

How I Did it

How to verify it

Description for the Changelog

Picture of a Boat (not required but encouraged)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebramanti Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

dexhorthy left a comment

Choose a reason for hiding this comment

ebramanti commented Aug 17, 2018 •

edited

Loading

ebramanti Aug 20, 2018 •

edited

Loading