Update user.toml path and example images #172

asymmetric · 2018-01-30T15:08:57Z

This PR updates the images used in examples. The new images feature an updated supervisor, which looks for the user.toml file in a new location.

Also, this PR changes the example to use redis whenever possible, to reduce maintenance overhead.

Closes #164.
Closes #165.

lilic · 2018-01-30T15:29:01Z

I would like to see a test for configuration updates as part of this PR, wdyt?

asymmetric · 2018-01-31T11:20:25Z

@lilic what kind of tests were you thinking of?

lilic · 2018-01-31T11:28:10Z

@asymmetric Similar as we already have for initial config, but this to make sure config updates work and will always work despite the fact we change something.

asymmetric · 2018-01-31T11:42:56Z

I'd keep that for a separate PR. I don't see a very strong correlation between this PR and tests for user.toml updates.

Also, this PR is blocking the release. Or do you mean that we should have the tests before we release?

Either way, I'd keep the two things separate.

krnowak

Looks good I think.

krnowak · 2018-01-31T11:53:05Z

examples/config/habitat.yml

-  # expects that encoding.
-  # Plain text content: protected-mode = "no"
-  user.toml: cHJvdGVjdGVkLW1vZGUgPSAibm8i
+  # Each item needs to be base64-encoded in base64.


The reason why I went with the protected-mode, instead of port number was that we can see a different message when visiting <cluster_ip>:30001. With protected mode set by default to "yes", we get a message about Redis being locked down. With our user.toml override, we could see something else.

But in the end, changing the port number is also fine - you documented that below in the Service part.

krnowak · 2018-01-31T11:56:54Z

examples/config/README.md

@@ -1,6 +1,6 @@
 # Initial configuration

-This example demostrates how initial configuration works with the Habitat operator. With the manifest file we deploy a Redis Habitat service.
+This example demonstrates how initial configuration works with the Habitat operator. With the manifest file we deploy a Redis Habitat service.


I'd be against fixing the typo - the word with the typo sounds like a Greek name and we need some greek reference, because Kubernetes.

I'm actually against it but for a very different reason: it's not relevant to this patch. :)

Oh I see that it is relevant, cause such a vague/broad commit log verb. :D

krnowak · 2018-01-31T12:01:32Z

Filed an issue for adding a test for config updates, so we won't forget that.

zeenix · 2018-01-31T12:46:12Z

examples/bind-config/habitat.yml

  user.toml: cG9ydCA9IDQ0NDQ=
 ---
 apiVersion: habitat.sh/v1
-kind: Habitat 
+kind: Habitat


irrelevant change. Separate patch for fixing formatting please

zeenix · 2018-01-31T12:47:07Z

examples/bind/README.md

@@ -10,6 +10,6 @@ After the Habitat operator is up and running, execute the following command from
 kubectl create -f examples/bind/habitat.yml
 ```

-This will deploy two `Habitat`s, a simple HTTP server written in Go that will be bound to a PostgreSQL database. The Go server will display the port number the database listens on.
+This will deploy two `Habitat`s, a simple HTTP server written in Go that will be bound to a Redis database. The Go server will display the port number the database listens on.


Do we really call the services or apps as "habitats"?

Yeah, the naming is a bit weird. In Habitat they're services, but the CRD is Habitat, so same as we say ConfigMaps, I think we're justified to say Habitats.

Not sure I agree and it sounds strange but OK. :)

I agree that it sounds strange, but this was the result of a long discussion and we couldn't come up with a better name.

See this and this.

zeenix · 2018-01-31T12:51:26Z

examples/bind-config/README.md

+bound to a Redis instance. By default, the Redis database instance would [listen
+on port
+6379](https://github.com/habitat-sh/core-plans/blob/7bc934c31e92c959aea0444671900c57c23d5265/redis/default.toml#L3),
+but we change this with the configuration stored in the `user-toml`.


in the commit log it's "user.toml", but here it's with a dash.

It's with a dash because what I'm pointing at here is the Kubernetes Secret named user-toml, rather than the filename under which the Secret is mounted in the Pod.

zeenix

I don't get the log message " Remove stutter", what's stutter?

krnowak · 2018-01-31T12:54:22Z

pkg/controller/controller.go

@@ -715,7 +715,7 @@ func (hc *HabitatController) newDeployment(h *habv1.Habitat) (*appsv1beta1.Deplo
 			Name: initialConfigFilename,
 			// The Habitat supervisor creates a directory for each service under /hab/svc/<servicename>.
 			// We need to place the user.toml file in there in order for it to be detected.
-			MountPath: fmt.Sprintf("/hab/svc/%s/%s", h.Spec.Service.Name, userTOMLFile),
+			MountPath: fmt.Sprintf("/hab/user/%s/config/%s", h.Spec.Service.Name, userTOMLFile),
 			SubPath:   userTOMLFile,


We need to get rid of this line.

krnowak

The SubPath thing should be dropped.

zeenix

As you know, I'm usually pretty big on atomic commits but I would have put all "postgresql -> redis" changes in one commit as it seems very much like one logical change, even if a big one affecting different files.

zeenix · 2018-01-31T13:02:03Z

examples/encrypted/README.md

-
-```
-  kubectl delete secret your-secret-name
-```


but we are not teaching k8s, just giving them a direct command to use so they don't have to look it up just for this.

Yeah but this is basic stuff, and it's also orthogonal: create/delete work across all resources. If you don't know how to do CRUD operations on a resource, I don't think you should delve into operators yet..

But I mean, I can add it back.. I just think it's useless, as virtually 100% of people reading this will know how to do it.

asymmetric · 2018-01-31T13:02:21Z

@zeenix I've first encountered stuttering as a naming anti-pattern here, it basically means that names should not repeat: node.Node would be stutter, buffer.BufferAt.

In this case, a Service whose name ends in -service, and stuff like that.

Some more info.

zeenix · 2018-01-31T13:05:08Z

examples/bind-config/habitat.yml

@@ -1,7 +1,7 @@
 apiVersion: v1
 kind: Secret
 metadata:
-  name: user-toml-secret
+  name: user-toml


Thanks for explaining what "stutter" is but i don't see any repetition here in the name.

In this case, a Service whose name ends in -service, and stuff like that.

Or a Secret whose name ends in -secret.

kubectl get secret returning user-toml-secret is stutter IMO. Of course it's a Secret. It doesn't help to have -secret there, because that object will only be returned when you ask for Secrets.

I agree with the change. I was just being nitpicky about it not fitting the "stutter" title.

krnowak

One line to remove.

krnowak · 2018-01-31T13:54:45Z

examples/config/README.md

-This will create a [Kubernetes Secret](https://kubernetes.io/docs/concepts/configuration/secret/) with the configurations and a simple Node.js application that will display a msg. When running on minikube, it can be accessed under port `30001` of the minikube VM. `minikube ip` can be used to retrieve the IP.
-Initially our app is configured to display the msg `"Hello world."`. Because we override this with the Secret we just created, our app will instead display `Hello from our Habitat-Operator!`.
+This will create a [Kubernetes Secret](https://kubernetes.io/docs/concepts/configuration/secret/) with the configurations and a Redis database.
+Initially the Redis database is configured to be in protected mode. Because we override this with the Secret we just created, our db will not be in this mode anymore.


Drop this line, please - you decided to change the port instead of the protected mode.

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Changing the port is a simpler, clearer example. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Make the filename independent of the implementation. In case we want to switch db or web app again in the future. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

It's unnecessary. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

No need to teach people how to use Kubernetes. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

The path was changed in habitat-sh/habitat#3814 Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

The new path only contains the user.toml file, so the whole-directory can be bind-mounted, without having to resort to the symlinking hacks of SubPath. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

We now change the port, instead of the protected mode. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

It had been removed by mistake in fa18258. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

It was creating a directory named user.toml and placing the user.toml there. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

krnowak

Heh, you also forgot about removing "user.toml" from the mount path, so you got user.toml directory inside config? Happened to me too in #164. ;)

Anyway, LFAD.

asymmetric requested review from zeenix, krnowak and lilic January 30, 2018 15:08

krnowak approved these changes Jan 31, 2018

View reviewed changes

krnowak mentioned this pull request Jan 31, 2018

Test config updates #174

Closed

zeenix reviewed Jan 31, 2018

View reviewed changes

krnowak reviewed Jan 31, 2018

View reviewed changes

krnowak suggested changes Jan 31, 2018

View reviewed changes

zeenix reviewed Jan 31, 2018

View reviewed changes

asymmetric force-pushed the krnowak/redis branch from 70cc1bf to 8023360 Compare January 31, 2018 13:20

krnowak suggested changes Jan 31, 2018

View reviewed changes

asymmetric force-pushed the krnowak/redis branch from a73e55c to 5f56ca3 Compare January 31, 2018 14:22

krnowak and others added 7 commits January 31, 2018 15:39

Use redis package instead of various multiple packages

fa18258

Capitalize Redis

b0dfc91

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Clarify how the user.toml config changes port

b54e72b

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Remove stutter

0130809

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Change port in example

89c1da3

Changing the port is a simpler, clearer example. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Fix remaining reference to postgreql in binding

66f5cc1

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Rename test files

a2f36b9

Make the filename independent of the implementation. In case we want to switch db or web app again in the future. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Lorenzo Manacorda added 10 commits January 31, 2018 15:39

Remove references to postgresql in tests

b3b1b98

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Remove suffix from test example names

0b7b529

It's unnecessary. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

General text readability improvements

ef8756d

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Explain port change

09f16b5

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Remove instructions on how to delete Secret

26c37c2

No need to teach people how to use Kubernetes. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Update mount path for user.toml

9d2293e

The path was changed in habitat-sh/habitat#3814 Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Remove SubPath in mount

4c528d7

The new path only contains the user.toml file, so the whole-directory can be bind-mounted, without having to resort to the symlinking hacks of SubPath. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Remove incorrect line from README

bb8593a

We now change the port, instead of the protected mode. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Add missing Kind to example

98a0160

It had been removed by mistake in fa18258. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Remove subdir from mount path

4e36d97

It was creating a directory named user.toml and placing the user.toml there. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

asymmetric force-pushed the krnowak/redis branch from 5f56ca3 to 4e36d97 Compare January 31, 2018 15:36

krnowak approved these changes Jan 31, 2018

View reviewed changes

krnowak merged commit ed7ca80 into master Jan 31, 2018

krnowak deleted the krnowak/redis branch January 31, 2018 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update user.toml path and example images #172

Update user.toml path and example images #172

asymmetric commented Jan 30, 2018 •

edited

Loading

lilic commented Jan 30, 2018

asymmetric commented Jan 31, 2018

lilic commented Jan 31, 2018

asymmetric commented Jan 31, 2018

krnowak left a comment

krnowak Jan 31, 2018

krnowak Jan 31, 2018

zeenix Jan 31, 2018

zeenix Jan 31, 2018

krnowak commented Jan 31, 2018

zeenix Jan 31, 2018

zeenix Jan 31, 2018

asymmetric Jan 31, 2018

zeenix Jan 31, 2018

asymmetric Jan 31, 2018

zeenix Jan 31, 2018

asymmetric Jan 31, 2018 •

edited

Loading

zeenix left a comment

krnowak Jan 31, 2018

krnowak left a comment

zeenix left a comment

zeenix Jan 31, 2018

asymmetric Jan 31, 2018

asymmetric Jan 31, 2018

asymmetric commented Jan 31, 2018 •

edited

Loading

zeenix Jan 31, 2018

asymmetric Jan 31, 2018 •

edited

Loading

asymmetric Jan 31, 2018

zeenix Jan 31, 2018

krnowak left a comment

krnowak Jan 31, 2018

krnowak left a comment

Update user.toml path and example images #172

Update user.toml path and example images #172

Conversation

asymmetric commented Jan 30, 2018 • edited Loading

lilic commented Jan 30, 2018

asymmetric commented Jan 31, 2018

lilic commented Jan 31, 2018

asymmetric commented Jan 31, 2018

krnowak 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

krnowak commented Jan 31, 2018

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

asymmetric Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

zeenix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak left a comment

Choose a reason for hiding this comment

zeenix 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

asymmetric commented Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

asymmetric Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak left a comment

Choose a reason for hiding this comment

asymmetric commented Jan 30, 2018 •

edited

Loading

asymmetric Jan 31, 2018 •

edited

Loading

asymmetric commented Jan 31, 2018 •

edited

Loading

asymmetric Jan 31, 2018 •

edited

Loading