Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST api - "env" section seems to be structured differently than other key value pair attributes #5490

Closed
abonas opened this issue Mar 15, 2015 · 7 comments
Labels
area/api Indicates an issue on api area. area/usability kind/support Categorizes issue or PR as a support question. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@abonas
Copy link
Contributor

abonas commented Mar 15, 2015

The "env" section in containers in pod has a different structure than other key/value structures as labels.
For example, labels is a hash with key:value pairs.
"labels": {
"role": "pod",
"mylabel": "mylabelvalue"
}

But "env" for some reason is an array of hashes where there are always 2 key names hardcoded:

"env": [
{
"name": "STORAGE",
"value": "local"
},
{
"name": "STORAGE_PATH",
"value": "/var/lib/docker-registry"
}
]

Why are the "name" and the "value" attributes required and making this an array of hashes, rather than making it like labels a single hash with key:value structure without hardcoding name/value attribute names?
for example:
the current:
"name": "STORAGE_PATH",
"value": "/var/lib/docker-registry"
can become:
"STORAGE":"local"

It will make it consistent with other key value pairs in the project, but will also reduce dramatically the json size when it comes to many containers per pod.

@abonas
Copy link
Contributor Author

abonas commented Mar 15, 2015

cc @oschreib

@a-robinson a-robinson added priority/backlog Higher priority than priority/awaiting-more-evidence. team/master labels Mar 16, 2015
@bgrant0607
Copy link
Member

Env is a list.

Long-debated topic.

https://github.com/GoogleCloudPlatform/kubernetes/blob/master/docs/api-conventions.md#lists-of-named-subobjects-preferred-over-maps
#2004
#4889

@ghodss Any preference for list vs. map with respect to Env specifically?

@bgrant0607 bgrant0607 added area/api Indicates an issue on api area. area/usability sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. kind/support Categorizes issue or PR as a support question. priority/support and removed team/master priority/backlog Higher priority than priority/awaiting-more-evidence. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. labels Mar 16, 2015
@ghodss
Copy link
Contributor

ghodss commented Mar 16, 2015

Labels are represented internally as a map, so it doesn't suffer from all the issues of trying to represent a list of subobjects as a map. EnvVars are currently represented as objects, so they need to be a list. If they were fully converted to be a map[string]string and not a list of subobjects, then there's no issue turning the API representation into a map as well.

One question we should ask ourselves before converting to a map is, will env vars ever need additional attributes or metadata on a per-env-var basis? Maybe you could indicate that a given env var is encrypted, or some other metadata that you want to associate. That seems unlikely, and Docker even represents it just as a list of strings, so unless someone can think of something I would not be opposed to removing the EnvVar object and changing EnvVars to be maps both internally and in the API.

This does have the negative effect of having more YAML keys that don't represent API fields (discussed in #2004 (comment)), but if we're going to be strict on that, then labels need to be converted to a list of Label objects as well, which I personally am not opposed to, but I don't know if anyone would get on board with that.

@bgrant0607
Copy link
Member

@thockin reminded me of the proposal to add a flag to each env. var. that indicates whether expansion should be performed: #560 (comment)

The alternative would be 2 separate lists/maps.

I don't think we're going to have a 100% consistent policy about lists vs. maps.

@ghodss
Copy link
Contributor

ghodss commented Apr 24, 2015

We should make a call on this, at least for the scope of v1 API, and close the issue. My bias is to leave it as is, but if others feel strongly we can change it to a map.

@thockin
Copy link
Member

thockin commented Apr 24, 2015

There's a PR in flight to add a third field, making this no long
map[string]string eligible. I say close this as WAI

On Fri, Apr 24, 2015 at 4:31 PM, Sam Ghods notifications@github.com wrote:

We should make a call on this, at least for the scope of v1 API, and close
the issue. My bias is to leave it as is, but if others feel strongly we can
change it to a map.


Reply to this email directly or view it on GitHub
#5490 (comment)
.

@bgrant0607
Copy link
Member

FYI, that PR is #6739.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. area/usability kind/support Categorizes issue or PR as a support question. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

5 participants