-
Notifications
You must be signed in to change notification settings - Fork 213
Replace ClusterVersions CRD with openshift/api artifact #266
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
Conversation
0920c11 to
efb0a6f
Compare
|
This LGTM |
| submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds' | ||
| type: string | ||
| metadata: | ||
| description: ObjectMeta is metadata that all persisted resources must have, |
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.
$ git diff -M20 origin/pr/266^^..origin/pr/266
...
metadata:
- description: ObjectMeta is metadata that all persisted resources must have,
- which includes all objects users must create.
- properties:
...Where did the metadata details go? Don't we want those?
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.
@wking no, those were removed in openshift/api#478 to conform to structural schema requirements
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.
... to conform to structural schema requirements
Can you link me to docs on these requirements? I don't see anything that sounds like it here.
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.
In the structural schema KEP the section on metadata describes:
The metadata field at the object root is implicitly specified and validated. In order to enforce that every CustomResource is a good citizen in the API Machinery of the API server, i.e. that features like owner references, finalizers, server-side apply etc. work as designed, we do not want that CRDs restrict metadata fields other than name and generateName.
So, we shouldn't be restricting description in the CRD for metadata
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.
Metadata is exclusively owned by the apiserver and apimachinery. A schema inside a CRD will interfere with apimachinery and we cannot guarantee that a CR from today works tomorrow.
Metadata is validated, but implicitly following the logic inside the kube-apiserver.
soltysh
left a comment
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.
/lgtm
|
@soltysh: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| metadata: | ||
| type: object | ||
| spec: | ||
| description: spec hold the intent of how this operator should behave. |
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've filed openshift/api#499 to fix this spec hold the typo.
| type: object | ||
| status: | ||
| description: status holds the information about the state of an operator. It | ||
| is consistent with status information across the kube ecosystem. |
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've filed openshift/api#500 expanding kube -> Kubernetes.
| versions: | ||
| description: versions is a slice of operand version tuples. Operators | ||
| which manage multiple operands will have multiple entries in the array. If | ||
| an operator is Available, it must have at least one entry. You must |
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.
Even operators with a single operand will have multiple entries, one for operator and one for the operand. And when you're Available, you need to have an operator entry, not just "an entry". I've filed openshift/api#501 to wordsmith that sort of thing.
|
/approve |
|
/lgtm We can always bump the vendored dir again if/when my API PRs land. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, soltysh, sttts, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, soltysh, sttts, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.