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

🐛 CRD generation: remove status before writing #630

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

justinsb
Copy link
Contributor

It's tricky to get the existing CRD types to not include the status,
so we instead allow for filtering of the map just before writing.

Issue #456

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 17, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 17, 2021
@justinsb justinsb force-pushed the strip_crd_status branch 2 times, most recently from 485e19d to 82dce5f Compare October 17, 2021 18:31
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2021
@justinsb
Copy link
Contributor Author

It's odd to include status for two reasons:

  1. We don't control the status; we control the spec.
  2. This seems to cause server-side apply to repeatedly reapply (on some k8s versions?)

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/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 Feb 12, 2022
@@ -147,6 +160,41 @@ func (g GenerationContext) WriteYAML(itemPath string, objs ...interface{}) error
return nil
}

// MarshalToYAML is based on sigs.k8s.io/yaml.Marshal, but allows for transforming the final data before writing.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted - thanks!

@yuwenma
Copy link

yuwenma commented Feb 25, 2022

This PR can greatly highlight and reemphasize the definition of object spec and status
and avoids the potential misuse of status which should be a control plane task to "continually and actively manages every object's actual state".

@yuwenma
Copy link

yuwenma commented Feb 25, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2022
@yuwenma
Copy link

yuwenma commented Feb 25, 2022

/assign @mengqiy

Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@mengqiy
Copy link
Member

mengqiy commented Feb 25, 2022

/hold
since the is a comment from Yuwen, please feel free to cancel the hold after addressing the comment.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, mengqiy

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2022
@mengqiy
Copy link
Member

mengqiy commented Feb 25, 2022

/retest

It's tricky to get the existing CRD types to not include the status,
so we instead allow for filtering of the map just before writing.

Issue kubernetes-sigs#456
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2022
@justinsb
Copy link
Contributor Author

Fixed comment - thanks for reviews @yuwenma and @mengqiy

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2022
@yuwenma
Copy link

yuwenma commented Feb 25, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2022
@k8s-ci-robot k8s-ci-robot merged commit 8cb5ce8 into kubernetes-sigs:master Feb 25, 2022
akutz added a commit to vmware-tanzu/vm-operator that referenced this pull request Nov 19, 2022
This patch updates hack/tools to use controller-tools 0.10.0.
In version 0.9+ of controller-tools, an upstream patch removed
the status sub-resource from the CustomResourceDefinition (CRD)
for a...CRD. That's right, the "kind: CustomResourceDefinition"
no longer has a status sub-resource, and that's why all of the
manifests have been touched during this patch. For more info,
please see
kubernetes-sigs/controller-tools#630.
akutz added a commit to vmware-tanzu/vm-operator that referenced this pull request Dec 1, 2022
This patch updates hack/tools to use controller-tools 0.10.0.
In version 0.9+ of controller-tools, an upstream patch removed
the status sub-resource from the CustomResourceDefinition (CRD)
for a...CRD. That's right, the "kind: CustomResourceDefinition"
no longer has a status sub-resource, and that's why all of the
manifests have been touched during this patch. For more info,
please see
kubernetes-sigs/controller-tools#630.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants