dhall-kubernetes icon indicating copy to clipboard operation
dhall-kubernetes copied to clipboard

Figure out really required keys?

Open f-f opened this issue 7 years ago • 12 comments

There is some cases in which we mark some fields as required, but they are not. We should somehow figure out if they are actually required only in some situations, and generate the files accordingly.

Some examples:

  • in io.k8s.api.apps.v1.DeploymentSpec, selector is marked as required, but Kubernetes is perfectly happy in accepting a Deployment without it (it will populate it based on spec.template.metadata.labels.
  • in io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta, we manually mark name as a required field (I think it's required if it's a top-level object), but Kubernetes is perfectly happy in accepting a Deployment whose spec.template.metadata has no name.

Any ideas?

f-f avatar Jun 02 '18 19:06 f-f

I'd preferably do this in an automated way.. but the swagger files just don't contain enough information at the moment. So I'm not sure yet what the best approach here would be.

arianvp avatar Jun 04 '18 09:06 arianvp

Agreed. The most robust way to fix this would be to have the upstream push to the swagger file at least the actual requirements that is possible to encode with the swagger type system.

For example, we have these kind of things in the swagger file:

"imagePullPolicy": {
  "description": "Image pull policy. One of Always, Never, IfNotPresent. Defaults to Always if :latest tag is specified, or IfNotPresent otherwise. Cannot be updated. More info: https://kubernetes.io/docs/concepts/containers/images#updating-images",
  "type": "string"
 }

So until we read the docstring we have no idea that this:

  • is an Enum and that has these three possible values
  • and has a default value

Both things are encodable in Swagger.

f-f avatar Jun 04 '18 10:06 f-f

But until we do this (I assume we'll have to look into Kube code) I guess the only way would be to make special rules that we add as we find out things (as we already do)

f-f avatar Jun 04 '18 10:06 f-f

There will be only so much that we can do. The Swagger spec is automatically generated from the Go types, as far as I know, and Go, for example, has no notion of enums, so that's why enums are a bit weird

arianvp avatar Jun 05 '18 14:06 arianvp

Unrelated quesiton, but not sure where to ask otherwise. @f-f are you on any kind of chat medium where we can casually discuss stuff?

arianvp avatar Jun 12 '18 09:06 arianvp

@arianvp Sure, I have open messages on Twitter

f-f avatar Jun 12 '18 10:06 f-f

in io.k8s.api.apps.v1.DeploymentSpec, selector is marked as required, but Kubernetes is perfectly happy in accepting a Deployment without it (it will populate it based on spec.template.metadata.labels.

This is definitely not not true anymore. This behaviour was removed quite some time ago. (Before Deployment went into v1) https://github.com/dhall-lang/dhall-kubernetes/issues/78#issuecomment-530805968

arianvp avatar Sep 12 '19 13:09 arianvp

About name in ObjectMeta, the K8s api supports generating name for resources if generateName is provided and name is not provided, it is documented in the API Reference. So, I think it is better to leave it as Optional.

akshaymankar avatar Oct 09 '19 01:10 akshaymankar

Could you open a PR for that @akshaymankar ?

arianvp avatar Oct 19 '19 20:10 arianvp

Can we close this issue once my PR is merged? I think we have solved the original two questions (e.g. selector should really be required these days; it's not a bug. and name is really optional, though it is "required" in some cases. In this case optional sounds like the sane default)

arianvp avatar Oct 19 '19 21:10 arianvp

OpenAPI object properties are optional by default (unless specified in the required list). I feel like it would be nicer if dhall-openapi respected this and treated all properties as Optional by default. That would avoid needing to add in special cases as they are encountered. I think this is more in keeping with the philosophy described by @Gabriel439 in https://github.com/dhall-lang/dhall-kubernetes/pull/85#pullrequestreview-304232455.

WillSewell avatar Jan 06 '21 10:01 WillSewell

Sounds good to me.

On Wed, 6 Jan 2021, 11:18 Will Sewell, [email protected] wrote:

OpenAPI object properties are optional by default (unless specified in the required list). I feel like it would be nicer if dhall-openapi respected this and treated all properties as Optional by default. That would avoid needing to add in special cases as they are encountered. I think this is more in keeping with the philosophy described by @Gabriel439 https://github.com/Gabriel439 in #85 (review) https://github.com/dhall-lang/dhall-kubernetes/pull/85#pullrequestreview-304232455 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dhall-lang/dhall-kubernetes/issues/8#issuecomment-755213541, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEZNI3I3PY5JCFBMEKORJ3SYQ2HPANCNFSM4FC7IOKA .

arianvp avatar Jan 06 '21 10:01 arianvp