Ship init supports arbitrary upstreams #399

dexhorthy · 2018-08-20T13:39:18Z

What I Did

Updated how we resolve releases so that in addition to helm charts,
ship can pull an arbitrary directory of k8s manifests.

We now clone the repo into .ship, and then inspect if there's
a Chart.yaml. If there is, we moved the cloned repo path into ./chart,
and generate the values-kustomize lifecycle. If there's no Chart.yaml,
we move it straight to base, and use a minimal lifecycle to drop the user
straight into "kustomize".

This is mostly a POC, and could probably use some tweaks on the
UX side, esp around UI messages and their pacing/verbosity.

It could probably also use some unit testing love, but this has good
integration test coverage for now.

Resolves #268

How I Did it

We've add a new workflow for resolving releases, to try and consolidate
some of the various levels of abstraction in the specs.Resolver and in
kustomize.go. In order:

fetch upstream from github or using helm fetch
determine application type based on the upstream we fetched. For now, type is one of replicated.app, helm or k8s, but i think we wanna add knative soon
Write out upstream and content sha to state and to ShipAppMetadata
If helm, add helm chart metadata to ShipAppMetadata as well
If helm+fetch, write helm fetch parameters like --chart-version to state.json
generate a release based on application type + discovered metadata

There is a lot of stuff moving around in this PR with respect to "how
do we get the release we hand off to daemon/lifecycle", it may be easier to
read the just the new code first, before diving into the diff.

Changes in order of appearance:

refactor replicated.app spec fetching into specs/replicatedapp
refactor github client into specs/githubclient
new specs/apptype package for determining app type
move init_chart integration tests to init, which just allows for passing in arbitrary args like --chart-version etc.
rename chartURL to upstream everywhere
rename HelmChartMetadata to ShipAppMetadata, the this info is
pretty generic and can apply to non-helm charts
Create a .ship/tmp for temporary workspace stuff, to be cleaned up
when ship terminates. This lets components write to that dir, without
having to worry about how/when to do a deferred cleanup.
- Cobra commands do an os.MkdirAll on .ship/tmp before ship starts
- Cobra commands do an os.RemoveAll on .ship/tmp as ship shuts down
- On Shutdown, ship does an os.RemoveAll on .ship/tmp (this handles the case where cobra doesn't get to clean up, because ship did an os.Exit)
Rename some constants to be more generic for non-helm apps
use BackupIfPresent before removing any directories, move the
function into a utililty package, add a terminal warning when backing up
directories.
Rework specs.Resolver for the new workflow above
expand determineApplicationType to handle raw k8s case, as well as the old helm fetch case
add methods to migrate state.ChartURL to state.Upstream cleanly.
helm asset uses .ship/tmp/.helm as HELM_HOME

Other fun stuff:

integration tests pretty-print json before diffing
state.Manager saves pretty-printed state json
Increase timeout from 60ms to 120ms on a flaky test
More fixes for weird tmpdir stuff on osx

How to verify it

Check out integration tests, but you can now do ship init for repo paths that
have no helm chart, just k8s yaml. For example, you can try out my fork of
sysdig cloud's kubernetes installer:

ship init github.com/dexhorthy/sysdigcloud-kubernetes/sysdigcloud

Description for the Changelog

Support fetching/kustomizing an arbitary directory of Kubernetes manifests with ship init.

Special thanks to @jadengore who helped out on debugging and fixing a few things here,
and whose commits got obliterated in what turned out to be a pretty messy rebase after
the changes for helm fetch.

dexhorthy · 2018-08-20T13:48:34Z

pkg/lifecycle/daemon/routes_navcycle_completestep_test.go

@@ -156,7 +156,7 @@ func TestV2CompleteStep(t *testing.T) {
 			POST: "/api/v1/navcycle/step/make-the-things",
 			// need to wait until the async task completes before we check all the expected mock calls,
 			// otherwise the state won't have been saved yet
-			WaitForCleanup: func() <-chan time.Time { return time.After(60 * time.Millisecond) },
+			WaitForCleanup: func() <-chan time.Time { return time.After(120 * time.Millisecond) },


this change is unrelated, just trying to get this test to flake less

dexhorthy · 2018-08-20T13:49:07Z

pkg/constants/filepaths.go

+
+// ShipPathInternal is the default folder path of Ship configuration
+var ShipPathInternalTmp = path.Join(".ship", "tmp")
+var ShipPathInternalTmpHelmHome = path.Join(ShipPathInternalTmp, ".helm")


renamed some of these paths, and modified them to use path.Join instead of /

dexhorthy · 2018-08-20T13:49:59Z

pkg/lifecycle/render/backup.go

-	"github.com/go-kit/kit/log/level"
-	"github.com/pkg/errors"
-	"github.com/spf13/afero"
+	"github.com/replicatedhq/ship/pkg/util"


moved backup logic into util package

dexhorthy · 2018-08-20T13:50:11Z

pkg/lifecycle/render/dockerlayer/layer_test.go

-					archiver.EXPECT().Open(&matchers.StartsWith{Value: "/tmp/dockerlayer"}, &matchers.StartsWith{Value: "/tmp/dockerlayer"}).Return(nil)
-					archiver.EXPECT().Open(&matchers.StartsWith{Value: "/tmp/dockerlayer"}, asset.Dest).Return(nil)
+					archiver.EXPECT().Open(&matchers.Contains{Value: "/dockerlayer"}, &matchers.Contains{Value: "/dockerlayer"}).Return(nil)
+					archiver.EXPECT().Open(&matchers.Contains{Value: "/dockerlayer"}, asset.Dest).Return(nil)


fix for tmpdir stuff

dexhorthy · 2018-08-20T13:50:55Z

pkg/lifecycle/render/helm/commands.go

@@ -6,15 +6,15 @@ import (

 // Commands are Helm commands that are available to the Ship binary.
 type Commands interface {
-	Init() error
+	Init(home string) error


I wanted to make this .ship/tmp/.helm, I know it probably collides with #396 , happy to let that go in and then resolve conflicts

dexhorthy · 2018-08-20T13:51:16Z

pkg/lifecycle/render/helm/template.go

@@ -98,7 +98,8 @@ func (f *LocalTemplater) Template(
 	templateArgs = append(templateArgs, args...)

 	debug.Log("event", "helm.init")
-	if err := f.Commands.Init(); err != nil {
+	// todo fix


waiting on #396

dexhorthy · 2018-08-20T13:54:36Z

pkg/ship/ship.go

 	}, nil
 }

 func (s *Ship) Shutdown(cancelFunc context.CancelFunc) {
+	s.FS.RemoveAll(constants.ShipPathInternalTmp)


This removes the tmp folder in the case that we're gonna os.Exit(1) due to failure, because in that case cobra wont get a change to clean it up

laverya · 2018-08-20T17:25:30Z

pkg/lifecycle/render/helm/template.go

@@ -98,7 +98,8 @@ func (f *LocalTemplater) Template(
 	templateArgs = append(templateArgs, args...)

 	debug.Log("event", "helm.init")
-	if err := f.Commands.Init(); err != nil {
+	// todo fix
+	if err := f.Commands.Init(".ship/tmp/.helm"); err != nil {


Shouldn't this refer to the constant?

laverya · 2018-08-20T17:26:17Z

pkg/lifecycle/render/helm/template_test.go

@@ -134,7 +134,7 @@ func TestForkTemplater(t *testing.T) {
 				},
 				optionAndValuesArgs...,
 			)
-			mockCommands.EXPECT().Init().Return(nil)
+			mockCommands.EXPECT().Init(".ship/tmp/.helm").Return(nil)


Same here - IMO this should refer to the helm tempdir constant, not to a literal string

laverya · 2018-08-20T17:50:55Z

pkg/ship/kustomize.go

-	"path/filepath"
+	"os"
+
+	"fmt"


5 stdlib imports
4 groups
lol

"goimports is good"

laverya · 2018-08-20T17:58:28Z

pkg/specs/apptype/determine_type.go

+	}
+
+	// "sure"
+	isGithub := strings.HasPrefix(strings.TrimLeft(upstream, "htps:/"), "github.com/")


Pretty sure this should be https
Not htps
Possibly also http

Also this could use a test or three

htps:/ is a "cutset", I agree this method could use a test, will probably add on in a follow up, since this is getting huge. https://play.golang.org/p/DeCvtN2dM7N

laverya

You couldn't have made this PR any bigger?
Anyways, I like the structure change
LGTM barring comments

@jadengore

What I Did ------------ Updated how we resolve releases so that in addition to helm charts, ship can pull an arbitrary directory of k8s manifests. We now clone the repo into .ship, and then inspect if there's a `Chart.yaml`. If there is, we moved the cloned repo path into `./chart`, and generate the values-kustomize lifecycle. If there's no `Chart.yaml`, we move it straight to `base`, and use a minimal lifecycle to drop the user straight into "kustomize". This is mostly a POC, and could probably use some tweaks on the UX side, esp around UI messages and their pacing/verbosity. It could probably also use some unit testing love, but this has good integration test coverage for now. How I Did it ------------ We've add a new workflow for resolving releases, to try and consolidate some of the various levels of abstraction in the `specs.Resolver` and in `kustomize.go`. In order: - fetch upstream from github or using `helm fetch` - determine application type based on the upstream we fetched. For now, type is one of `replicated.app`, `helm` or `k8s`, but i think we wanna add knative soon - Write out upstream and content sha to state and to ShipAppMetadata - If helm, add helm chart metadata to ShipAppMetadata as well - If helm+fetch, write helm fetch parameters like `--chart-version` to state.json - generate a release based on application type + discovered metadata There is a *lot* of stuff moving around in this PR with respect to "how do we get the release we hand off to daemon/lifecycle", it may be easier to read the flow of the new code, rather than trying to grok the diff. Changes in order of appearance: - refactor replicated.app spec fetching into specs/replicatedapp - refactor github client into specs/githubclient - new specs/apptype package for determining app type - move `init_chart` integration tests to `init`, which just allows for passing in arbitrary args like `--chart-version` etc. - rename `chartURL` to upstream everywhere - rename `HelmChartMetadata` to `ShipAppMetadata`, the this info is pretty generic and can apply to non-helm charts - Create a `.ship/tmp` for temporary workspace stuff, to be cleaned up when ship terminates. This lets components write to that dir, without having to worry about how/when to do a deferred cleanup. - Cobra commands do an os.MkdirAll on `.ship/tmp` before ship starts - Cobra commands do an os.RemoveAll on `.ship/tmp` as ship shuts down - On Shutdown, ship does an `os.RemoveAll` on `.ship/tmp` (this handles the case where cobra doesn't get to clean up, because ship did an os.Exit) - Rename some constants to be more generic for non-helm apps - use `BackupIfPresent` before removing any directories, move the function into a utililty package, add a terminal warning when backing up directories. - Rework specs.Resolver for the new workflow above - expand determineApplicationType to handle raw k8s case, as well as the old `helm fetch` case - add methods to migrate `state.ChartURL` to `state.Upstream` cleanly. - `helm` asset uses `.ship/tmp/.helm` as HELM_HOME Other fun stuff: - integration tests pretty-print json before diffing - `state.Manager` saves pretty-printed state json - Increase timeout from 60ms to 120ms on a flaky test - More fixes for weird tmpdir stuff on osx How to verify it ------------ Check out integration tests, but you can now do `ship init` for repo paths that have no helm chart, just k8s yaml. For example, you can try out [my fork of sysdig cloud's kubernetes installer](https://github.com/dexhorthy/sysdigcloud-kubernetes/tree/master/sysdigcloud): ``` ship init github.com/dexhorthy/sysdigcloud-kubernetes/sysdigcloud ``` Description for the Changelog ------------ Support fetching/kustomizing an arbitary directory of Kubernetes manifests with `ship init`. ![](https://upload.wikimedia.org/wikipedia/commons/thumb/a/a4/FrotamericaShipWreck.jpg/2560px-FrotamericaShipWreck.jpg) Special thanks to @jadengore who helped out on debugging and fixing a few things here, and whose commits got obliterated in what turned out to be a pretty messy rebase after the changes for `helm fetch`.

dexhorthy requested review from ebramanti and laverya August 20, 2018 13:39

dexhorthy commented Aug 20, 2018

View reviewed changes

dexhorthy mentioned this pull request Aug 20, 2018

Use temporary helm home dir #396

Merged

laverya reviewed Aug 20, 2018

View reviewed changes

laverya approved these changes Aug 20, 2018

View reviewed changes

dexhorthy merged commit 93d7148 into replicatedhq:master Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ship init supports arbitrary upstreams #399

Ship init supports arbitrary upstreams #399

dexhorthy commented Aug 20, 2018 •

edited

Loading

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018

dexhorthy Aug 20, 2018 •

edited

Loading

laverya Aug 20, 2018

laverya Aug 20, 2018

laverya Aug 20, 2018

dexhorthy Aug 20, 2018

laverya Aug 20, 2018 •

edited

Loading

laverya Aug 20, 2018

dexhorthy Aug 20, 2018

laverya left a comment

Ship init supports arbitrary upstreams #399

Ship init supports arbitrary upstreams #399

Conversation

dexhorthy commented Aug 20, 2018 • edited Loading

What I Did

How I Did it

How to verify it

Description for the Changelog

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

dexhorthy Aug 20, 2018 • edited Loading

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

laverya Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laverya left a comment

Choose a reason for hiding this comment

dexhorthy commented Aug 20, 2018 •

edited

Loading

dexhorthy Aug 20, 2018 •

edited

Loading

laverya Aug 20, 2018 •

edited

Loading