deis / charts

(OBSOLETE) Helm Classic v1 Charts for Deis Workflow
https://deis.com/workflow/
MIT License
45 stars 36 forks source link

feature(tpl): expose version string as annotation #331

Closed jackfrancis closed 8 years ago

deis-bot commented 8 years ago

@helgi, @krancour and @sgoings are potential reviewers of this pull request based on my analysis of git blame information. Thanks @jackfrancis!

krancour commented 8 years ago

I think we should consider prefixing the annotation with our "namespace" (sorry for the overloaded term) as we do in other places-- the router being one of them.

So the annotation would be <component>.deis.io/version

krancour commented 8 years ago

Also, I see this is being applied also to some manifests that use helm-keep: "true".

We should probably check with @mboersma...

Do "kept" manifests still get updated upon re-install? I cannot recall.

If so, then all is good. If not, the version annotation could get "stuck" at its original value from the initial installation. Just want to be sure that's not going to be the case.

jackfrancis commented 8 years ago

@krancour Thanks for chiming in. Can you advocate for the benefits of doing so? The annotation will already be literally namespaced (going out on a limb and using the correct form of the word "literally") inside its specific k8s api resource type. See the Annotations property here:

https://godoc.org/k8s.io/kubernetes/pkg/api#ObjectMeta

Any distinct resource of any type will have its own annotations dictionary; moreover, each resource will belong to the k8s namespace "deis".

Just trying to tease out the functional value here. (I appreciate the associative value of sticking to existing patterns, if that's the compelling case.)

krancour commented 8 years ago

This argument might be a bit academic, but suppose for a moment that all the annotations were just in a single flat, shared namespace (again, I apologize for the overloading, as I am not referring to a k8s namespace)...

Now imagine that some pod uses two different libraries that both, for their own distinct purposes, require use of an annotation x. There is then a conflict because you could not satisfy the requirements of both libraries.

A more plausible variation on the same scenario... two or more distinct agents, completely external to a given deployment, service, pod, etc. act upon the annotations on deployments, services, pods, etc. (The router, for instance, is such an agent.) While one agent may set or interpret annotation x in one way, another may set or interpret it differently-- so competing concerns arise once again.

For our part, we can avoid these scenarios by prefixing annotations with <component>.deis.io/. This is directly analogous with why Go uses VCS URLs in package names or why Java uses domains in package names-- to avoid conflict.

slack commented 8 years ago

Re: manifest updates, we rely on the semantics of kubectl apply.

I agree WRT namespacing configuration to the component name. But I feel that the version isn't configuration, rather an attribute. Which would argue for a consistent, concrete, key name across all components e.g.:

component.deis.io/version=v1.2.3 rather than router.deis.io/version=v1.2.5

Which would also let you do: component.deis.io/name=router

That way, WFM can generically process objects in the Kubernetes deis namespace without having to fiddle with object name to component name mappings.

krancour commented 8 years ago

@slack that seems pretty reasonable.

jackfrancis commented 8 years ago

@krancour @slack see latest squashed commit

krancour commented 8 years ago

This mostly LGTM, but now I am wondering if deis.io/componentVersion might read more cleanly.

@slack, thoughts?

"component" in component.deis.io/version sounds as if it is a component and there's no component named "component".

component

jackfrancis commented 8 years ago

@krancour May I suggest we proceed w/ component.deis.io/version in the absence of any objection?

jchauncey commented 8 years ago

I can see both sides here. Im fine with either.