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

⚠️ Only prevent validation of top-level ObjectMeta field #395

Closed

Conversation

arjunrn
Copy link
Contributor

@arjunrn arjunrn commented Jan 26, 2020

This addresses #385. Currently the validation for ObjectMeta is not generated even if it is used somewhere in the schema other than at the top-level. To fix this I mostly reverted the changes from #266 and then after the schema has been generated patched the top-level metadata to have no validation. The resulting schema for ObjectMeta is still big as @sttts mentions in the issue but I don't see how we can drop only the read-only fields. We can have a hand-written schema with only the required fields but this would need more effort in the long term keeping it in sync with upstream. I'm open to suggestions on how this can be improved.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 26, 2020
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 26, 2020
@arjunrn
Copy link
Contributor Author

arjunrn commented Jan 26, 2020

/assign @DirectXMan12

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arjunrn
To complete the pull request process, please assign directxman12
You can assign the PR to them by writing /assign @directxman12 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Arjun Naik <arjun.rn@gmail.com>
@pgier
Copy link
Contributor

pgier commented Feb 27, 2020

FWIW, this patch worked to fix our use case in prometheus-operator (prometheus-operator/prometheus-operator#3041)

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 27, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 26, 2020
@mcristina422
Copy link
Contributor

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jul 8, 2020
@DirectXMan12 DirectXMan12 changed the title ✨ Only prevent validation of top-level ObjectMeta field ⚠️ Only prevent validation of top-level ObjectMeta field Sep 11, 2020
@DirectXMan12
Copy link
Contributor

Title edit: this is a breaking change

@k8s-ci-robot
Copy link
Contributor

@arjunrn: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2020
@DirectXMan12
Copy link
Contributor

DirectXMan12 commented Sep 11, 2020

it almost sounds like we want format: x-kubernetes-metadata or something here, longterm

@DirectXMan12
Copy link
Contributor

I think this approach is fairly reasonable, but it is a breaking change. We should have a generator option to turn this on or off, I think. Please add that, and then this is good to go.

@DirectXMan12
Copy link
Contributor

(also, I realize this is super old at this point, so if the original submitter is no longer available and someone wants to take this over, please do)

@sttts
Copy link
Contributor

sttts commented Sep 14, 2020

I still think it is not a good idea to have full ObjectMeta in an embedded object. The technical work around is have some special EmbeddedObjectMeta with a restricted set of fields. I think that's the route that some operators went by now.

@DirectXMan12
Copy link
Contributor

What do we do about PodTemplateSpec, then?

@sttts
Copy link
Contributor

sttts commented Sep 22, 2020

What do we do about PodTemplateSpec, then?

Sins of the past.

Do we have any restriction on PodTemplateSpec in contrast to object-top-level meta data? We should have gone the same way there, i.e. EmbeddedObjectMeta.

@champak
Copy link

champak commented Sep 22, 2020

For operators that are already deployed re-organizing the CRD to add embeddedObjectMeta may not be feasible for an upgrade. If we could keep at least the name/namespace (typically these get referenced by other objects) of the ObjectMeta around post conversion, that would help. #448 Thanks

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 21, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 20, 2021
@dvaldivia
Copy link
Contributor

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 26, 2021
@arjunrn
Copy link
Contributor Author

arjunrn commented Apr 6, 2021

Closing in favor of #539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants