Adding THE KUBERNETES MIXIN #228

jeremyrickard · 2019-03-18T15:33:47Z

This PR adds a Kubernetes mixin. It updates the design doc a little to reflect a couple of
param changes across the three verbs and harmonizes things using camelCase naming per the
style guide from the GOOG.

Closes #82

carolynvs

not done yet, got as far as schema_test.go 😅

carolynvs · 2019-03-18T19:43:18Z

cmd/kubernetes/main.go

+	mixin.In = in
+	cmd := &cobra.Command{
+		Use:  "kubernetes",
+		Long: "kuberetes is a porter 👩🏽‍✈️ mixin that you can you can use to leverage kubernetes manifests",


nit: maybe replace leverage with "apply"

e.g. "apply kubernetes manifests in your bundle

carolynvs · 2019-03-18T19:44:41Z

cmd/kubernetes/main.go

+		PersistentPreRun: func(cmd *cobra.Command, args []string) {
+			mixin.Out = cmd.OutOrStdout()
+			mixin.Err = cmd.OutOrStderr()
+		},


We've also figured out that we want SilenceUsage: true, here as well. Otherwise it prints the help text anytime the an error happens.

carolynvs · 2019-03-18T19:45:34Z

cmd/kubernetes/uninstall.go

+func buildUnInstallCommand(mixin *kubernetes.Mixin) *cobra.Command {
+	return &cobra.Command{
+		Use:   "uninstall",
+		Short: "Use kubectl to delete manifests from cluster",


nit: delete resources contained in a manifest from a cluster

carolynvs · 2019-03-18T19:46:01Z

cmd/kubernetes/upgrade.go

+func buildUpgradeCommand(mixin *kubernetes.Mixin) *cobra.Command {
+	return &cobra.Command{
+		Use:   "Upgrade",
+		Short: "Use kubectl to apply manifests to cluster",


nit: to a cluster

carolynvs · 2019-03-18T19:47:54Z

docs/content/design/kubernetes-mixin.md

-        resource_type: "service"
-        resource_name: "super-cool-service"
-        jsonpath: "spec.clusterIP"
+        resourceType: "service"


Question on the switch from snakes to camels. Is that something that should be done for all outputs? Or because of k8s? I'm not quite clear on why you made the switch.

Just for consistency.

carolynvs · 2019-03-18T19:49:53Z

pkg/kubernetes/install.go

+		}
+		err = cmd.Wait()
+		if err != nil {
+			prettyCmd := fmt.Sprintf("%s %s", cmd.Path, strings.Join(cmd.Args, " "))


I think we are missing a if m.Debug and a print of the pretty command?

oh wait I just realized that the mixins don't do that. Ignore me! 😊

carolynvs · 2019-03-18T19:51:11Z

pkg/kubernetes/install_test.go

+
+func TestMixin_InstallStep(t *testing.T) {
+
+	manifestDirectory := "/cnab/app/manifesto"


is this a typo? I thought the directory was /cnab/app/manifests/?

It was, however it's gone now!

carolynvs · 2019-03-18T19:53:02Z

pkg/kubernetes/kubernetes.go

+	return data, errors.Wrap(err, "could not read payload from STDIN")
+}
+
+func (m *Mixin) ValidatePayload(b []byte) error {


Do you want to call this in getPayloadData or something so that we validate by default? Or hold off for a while?

carolynvs · 2019-03-18T19:55:18Z

pkg/kubernetes/kubernetes.go

+	"github.com/xeipuuv/gojsonschema"
+)
+
+const defaultManifestPath = "/cnab/app/manifests/kubernetes"


Is the plan for this that someone could do this below and it would automatically look in this location?

install: - kubernetes

I don't want to assume, but if my guess is right can you provide me a bit more context about that decision?

carolynvs · 2019-03-18T19:57:39Z

pkg/kubernetes/schema/kubernetes.json

+              },
+              "namespace": {
+                "type": "string",
+                "minLength": 0


Is there a difference (jsonschema speaking) between specifying minLength = 0 and leaving it off entirely?

carolynvs

ok all done, mega PR reviewed! Nothing big found, just minor comments.

carolynvs · 2019-03-18T20:55:14Z

pkg/kubernetes/testdata/install-input-no-manifests.yaml

+install:
+  - kubernetes:
+      description: "Install Hello World App"
+      manifests:


Is this test file correct? It doesn't seem to match the name because there are manifests specified.

carolynvs · 2019-03-18T20:56:08Z

pkg/kubernetes/uninstall.go

+}
+
+// UnInstall will delete anything created during the install or upgrade step
+func (m *Mixin) UnInstall() error {


typo: UnInstall -> Uninstall

carolynvs · 2019-03-18T20:57:33Z

pkg/kubernetes/uninstall_test.go

+
+func TestMixin_UninstallStep(t *testing.T) {
+
+	manifestDirectory := "/cnab/app/manifesto"


Again, not sure if this is a typo or a joke? 😀

carolynvs · 2019-03-19T16:47:16Z

pkg/kubernetes/install_test.go

@@ -23,7 +23,7 @@ func TestMain(m *testing.M) {

 func TestMixin_InstallStep(t *testing.T) {

-	manifestDirectory := "/cnab/app/manifesto"
+	manifestDirectory := "/cnab/app/manifests"


I'm relieved we don't have a manifesto anymore. 😅

carolynvs · 2019-03-19T16:48:20Z

pkg/kubernetes/schema/kubernetes.json

                "default":"true"
              },
              "record": {
-                "type": "boolean",
+                "type": ["boolean","null"],


what does it mean for the type to be bool or null?

This PR adds a Kubernetes mixin. It updates the design doc a little to reflect a couple of param changes across the three verbs and harmonizes things using camelCase naming per the [style guide](https://google.github.io/styleguide/jsoncstyleguide.xml?showone=Property_Name_Format#Property_Name_Format) from the GOOG. Closes getporter#82

ghost assigned jeremyrickard Mar 18, 2019

ghost added the review label Mar 18, 2019

jeremyrickard requested review from vdice and carolynvs March 18, 2019 15:34

carolynvs reviewed Mar 18, 2019

View reviewed changes

carolynvs approved these changes Mar 19, 2019

View reviewed changes

jeremyrickard force-pushed the kubing-all-those-netes branch from 22a7e40 to f4a8eb2 Compare March 19, 2019 16:53

Updated with code review comments

3cb85bf

jeremyrickard force-pushed the kubing-all-those-netes branch from f4a8eb2 to 3cb85bf Compare March 19, 2019 18:07

carolynvs merged commit 1bf86a1 into getporter:master Mar 19, 2019

ghost removed the review label Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding THE KUBERNETES MIXIN #228

Adding THE KUBERNETES MIXIN #228

jeremyrickard commented Mar 18, 2019 •

edited

Loading

carolynvs left a comment

carolynvs Mar 18, 2019

jeremyrickard Mar 19, 2019

carolynvs Mar 18, 2019

jeremyrickard Mar 19, 2019

carolynvs Mar 18, 2019

jeremyrickard Mar 19, 2019

carolynvs Mar 18, 2019

jeremyrickard Mar 19, 2019

carolynvs Mar 18, 2019

jeremyrickard Mar 19, 2019

carolynvs Mar 18, 2019

carolynvs Mar 19, 2019

carolynvs Mar 18, 2019

jeremyrickard Mar 19, 2019

carolynvs Mar 18, 2019

carolynvs Mar 18, 2019

carolynvs Mar 18, 2019

carolynvs left a comment

carolynvs Mar 18, 2019

carolynvs Mar 18, 2019

carolynvs Mar 18, 2019

carolynvs Mar 19, 2019

carolynvs Mar 19, 2019


		func TestMixin_InstallStep(t *testing.T) {

		manifestDirectory := "/cnab/app/manifesto"


		func TestMixin_UninstallStep(t *testing.T) {

		manifestDirectory := "/cnab/app/manifesto"

Adding THE KUBERNETES MIXIN #228

Adding THE KUBERNETES MIXIN #228

Conversation

jeremyrickard commented Mar 18, 2019 • edited Loading

carolynvs left a 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

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

carolynvs left a 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

jeremyrickard commented Mar 18, 2019 •

edited

Loading