search

habitat-sh / habitat-operator

A Kubernetes operator for Habitat services
Apache License 2.0
61 stars 17 forks source link

Define rationale for Habitat type structure #182

Open asymmetric opened 6 years ago

asymmetric commented 6 years ago

It's not totally clear at the moment why we put keys where we put them in the Habitat type.

I say this because I'm having trouble justifying why configSecretName should be under Service, rather than under Habitat directly.

Also, every time we have to add a new field, we should clearly know where.

I thought a clear rationale was: every field that represents a flag passed to the supervisor should go under Service. So for example: --name, --topology, --bind.

If that's true, than configSecretName should be move up.

But I wonder if such a rationale makes sense from the point of view of a user. It is not based on a logical grouping, rather it's based on inheritance (of the flags used by an existing tool).

asymmetric commented 6 years ago

I think we probably don't need to have a service key at all, i.e. we can just one level of depth.

asymmetric commented 6 years ago

@zeenix WDYT? Flattening the type layout will be a breaking change, and you seem to have strong feelings towards backwards compatibilty.

Also /cc @krnowak @LiliC

lilic commented 6 years ago

@asymmetric Can you paste a sample manifest file of your idea for better clarity.

Also as a side note according to the API version we are in beta not alpha, so these changes should have been done before that.

asymmetric commented 6 years ago

Basically,

apiVersion: habitat.sh/v1beta1
kind: Habitat
metadata:
  name: example-configured-habitat
spec:
  image: kinvolk/redis-hab
  count: 1
  service-name: db
  topology: standalone
  group: redisdb
  configSecretName: user-toml
zeenix commented 6 years ago

well I'm against needless API breakage but I'm not sure it's the case in here. However, we should try our best to design this well right now if we're breaking again and also consult Chef folks about breaking this API and how it affects the k8s and helm exporters.

asymmetric commented 6 years ago

After thinking about this some more, I think the current structure is fine, and that configSecretName is just an outlier that is probably misplaced

Basically, the "rationale" IMO is that spec.service is for flags that are passed to the operator. In this sense, configSecretName should've been placed directly under the spec key.

I've opened an issue to investigate whether that makes sense.

krnowak commented 6 years ago

As I said in #269, I think that a rationale could be k8s specific stuff vs habitat specific stuff.