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

Figure out really required keys? #8

Open
f-f opened this issue Jun 2, 2018 · 12 comments
Open

Figure out really required keys? #8

f-f opened this issue Jun 2, 2018 · 12 comments

Comments

@f-f
Copy link
Member

f-f commented Jun 2, 2018

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 f-f changed the title Figure out really required names? Figure out really required keys? Jun 2, 2018
@arianvp
Copy link
Member

arianvp commented Jun 4, 2018

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.

@f-f
Copy link
Member Author

f-f commented Jun 4, 2018

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
Copy link
Member Author

f-f commented Jun 4, 2018

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)

@arianvp
Copy link
Member

arianvp commented Jun 5, 2018

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
Copy link
Member

arianvp commented Jun 12, 2018

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?

@f-f
Copy link
Member Author

f-f commented Jun 12, 2018

@arianvp Sure, I have open messages on Twitter

@arianvp
Copy link
Member

arianvp commented Sep 12, 2019

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) #78 (comment)

@akshaymankar
Copy link

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.

@arianvp
Copy link
Member

arianvp commented Oct 19, 2019

Could you open a PR for that @akshaymankar ?

arianvp added a commit that referenced this issue Oct 19, 2019
It is only required for top-level objects .  (E.g. `Deployment`, `Pod`, `DaemonSet`) but it is Optional when it is part of a compond object
(e.g. `PodTemplateSpec` inside a `Deployment` or `DaemonSet`)


See #8 (comment)
and #84 (comment)
@arianvp
Copy link
Member

arianvp commented Oct 19, 2019

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)

@WillSewell
Copy link
Collaborator

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 #85 (review).

@arianvp
Copy link
Member

arianvp commented Jan 6, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants