-
Notifications
You must be signed in to change notification settings - Fork 173
Conversation
Partially implements the proposal [here](https://github.com/ksonnet/ksonnet/blob/master/design/proposals/explicit-environment-metadata.md) Fixes ksonnet#222 Signed-off-by: Jessica Yuen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a pretty much uncontroversial change, except for the one issue I bring up. What you're doing is pretty straightforward, and I appreciate you cleaning up the LibPaths
signature. :)
// | ||
|
||
params := importParams(string(paramsPath)) | ||
spec := importEnv(string(specPath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so, based on the comments in the design proposal, I assumed we were not exposing the server URI or the env name, and instead exposing just the namespace for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, I think this is the one point that I forgot to address in the proposal. The env spec.json
doesn't contain the name so we would only be exposing the server URI and the namespace.
If I recall correctly, your reservations around exposing the server URI was the if server == x else
problem. This is the same anti-pattern with namespace and I think it can be discouraged (as mentioned in our other discussion). I also think this pattern will be a lot less seen if name
isn't exposed.
Agree name
is also a ksonnet CLI specific attribute so it makes little sense to expose at the prototype level but server
and namespace
are Kubernetes attributes that can be useful as args to ksonnet-lib APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense to me. I'm going to mark as "approve", let's see what @bryanl thinks.
@bryanl Let me know if you have any additional thoughts before I merge later tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning LGTM.
Partially implements the proposal here.
Version 2 of prototypes in the parts library that follow the historical model of hard-coded namespaces will be updated subsequently. This change does not break existing prototypes.
Fixes #222
Signed-off-by: Jessica Yuen [email protected]