kubebuilder icon indicating copy to clipboard operation
kubebuilder copied to clipboard

🐛 Synchonized plural generation with api-machinery

Open lauchokyip opened this issue 2 years ago • 13 comments
trafficstars

Fixes https://github.com/kubernetes-sigs/kubebuilder/issues/3402

lauchokyip avatar May 27 '23 20:05 lauchokyip

Hi @lauchokyip. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 avatar May 27 '23 20:05 k8s-ci-robot

@lauchokyip

Could you please squash the commits ?

@varshaprasad96 @everettraven wdyt about this one? Could you please give a hand on the review?

camilamacedo86 avatar May 28 '23 17:05 camilamacedo86

/hold

Definitely feel like a breaking change

lauchokyip avatar May 30 '23 21:05 lauchokyip

Hi @lauchokyip,

Could you please squash the commits? Also, see that it is not passing in the lint you can run locally make lint locally to check it out.

By last, we need to provide a guidance for those that:

  • Scaffold the project with an previous version, create an api and then get a new kubebuilder version to create another API Could you please add the steps in the description of this PR? What guidance/steps should we provide in this case?

It seems for me that would not work at all and this one is a breaking change. For other hand it seems a valid / reasonable request. I am just trying to think if it could be addressed for v4 or if we should only perform this change for an future version like go/v5.

c/c @everettraven @varshaprasad96 @Kavinjsir wdyt about this one?

Hi @camilamacedo86 , do you mean providing the migration documents? Any guidance on how I could do that?

I don't know the code well enough to answer this question - if the user generate a CRD named Helmswoman and the plural is set to Helmswomen. Now, the user wants to generate another version for the same CRD for let's say v2, kind Helmswoman. When the new code is merged, would the plural be set to Helmswomans ? If the plural were to set to Helmswomans, will that break anything besides the generated file name?

I will be voting towards putting this into v5 but given the lack of knowledge and context of the source code, definitely need more inputs

lauchokyip avatar Jun 02 '23 02:06 lauchokyip

Hi @everettraven @varshaprasad96 @Kavinjsir @lauchokyip,

We have encountered several deprecations, and it's essential that we deprecate the declarative plugin as soon as possible. You can find the details in this pull request: https://github.com/kubernetes-sigs/kubebuilder/pull/3395

To ensure a cleaner and more maintainable codebase as we continue to grow, we plan to remove all deprecations at the beginning of next year, leading to a major bump for Kubebuilder 4.0.0.

However, since this change will have a significant impact on projects that are already scaffolded, I suggest we consider the following:

  • Let's carefully evaluate whether this change is necessary, taking into account the comments shared by @Kavinjsir in this pull request: https://github.com/kubernetes-sigs/kubebuilder/pull/3433#pullrequestreview-1453231505.
  • In the pull request, it would be helpful to document the manual steps required to update existing projects once this change is merged. @lauchokyip, could you please include these steps in the PR description? We need to provide guidance on how to modify projects built prior to this change, ensuring they won't be affected when a new version of Kubebuilder is released. Please specify the necessary file modifications and steps to be taken. (It is essential because it will help us properly document it in the release and migration guides)
  • @lauchokyip, please ensure that the new code is only used for go/v4 and doesn't impact go/v3, which is already deprecated. By doing so, when we eventually bump to 4.x, the condition will keep the code as the default.
  • Then, I think we might want to consider whether to keep this PR for future go/v5 changes. Once @lauchokyip implements the changes for impact only go/v4 as described above, we can modify the PR for go/v5 easily and merge it at the appropriate time.

Please share your thoughts on these suggestions.

camilamacedo86 avatar Jun 10 '23 07:06 camilamacedo86

@lauchokyip We are interested in resolving this issue. Could you please rebase this so that we revisit it again. Thanks for taking the time for working on this.

varshaprasad96 avatar Jul 27 '23 18:07 varshaprasad96

@lauchokyip We are interested in resolving this issue. Could you please rebase this so that we revisit it again. Thanks for taking the time for working on this.

Let me get back to this in a bit

lauchokyip avatar Jul 28 '23 04:07 lauchokyip

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: everettraven, lauchokyip Once this PR has been reviewed and has the lgtm label, please assign camilamacedo86 for approval. For more information see the Kubernetes Code Review Process.

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

k8s-ci-robot avatar Aug 05 '23 15:08 k8s-ci-robot

HI @varshaprasad96 , make generate are failing, can you please check it? I even git clone a brand new repo and it still failed

TODO: update tutorial
ERRO[0009] error fixing cronjob_webhook.go: unable to find the content to be replaced 
exit status 1
make: *** [Makefile:76: generate-docs] Error 1

Does make generate work for you?

lauchokyip avatar Aug 05 '23 15:08 lauchokyip

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 avatar Aug 09 '23 18:08 k8s-ci-robot

HI @varshaprasad96 , make generate are failing, can you please check it? I even git clone a brand new repo and it still failed

TODO: update tutorial
ERRO[0009] error fixing cronjob_webhook.go: unable to find the content to be replaced 
exit status 1
make: *** [Makefile:76: generate-docs] Error 1

Does make generate work for you?

@lauchokyip Would you rebase the branch based off the latest master branch? There might be an issue for make generate previously. The current master branch should have it fixed. Could you try and see if it works? Feel free to let me know if further questions rasied.

Kavinjsir avatar Aug 28 '23 19:08 Kavinjsir

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle stale

k8s-triage-robot avatar Jan 21 '24 12:01 k8s-triage-robot

@lauchokyip: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubebuilder-e2e-k8s-1-25-3 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-25-3
pull-kubebuilder-e2e-k8s-1-27-1 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-27-1
pull-kubebuilder-e2e-k8s-1-27-10 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-27-10
pull-kubebuilder-e2e-k8s-1-28-6 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-28-6
pull-kubebuilder-e2e-k8s-1-27-3 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-27-3
pull-kubebuilder-e2e-k8s-1-26-6 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-26-6
pull-kubebuilder-e2e-k8s-1-28-0 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-28-0
pull-kubebuilder-e2e-k8s-1-29-0 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-29-0
pull-kubebuilder-e2e-k8s-1-27-11 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-27-11
pull-kubebuilder-e2e-k8s-1-28-7 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-28-7
pull-kubebuilder-e2e-k8s-1-29-2 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-29-2
pull-kubebuilder-e2e-k8s-1-30-0 b31f4cf066bf4911012836aff79f5b55f7484613 link true /test pull-kubebuilder-e2e-k8s-1-30-0

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot avatar May 16 '24 13:05 k8s-ci-robot

Closed based on:

  • Huge breaking change https://github.com/kubernetes-sigs/kubebuilder/pull/3433#issuecomment-1585538375 without a good justification. Why the change is not proposed to apimachinery?
  • https://github.com/kubernetes-sigs/kubebuilder/issues/3402#issuecomment-2121490772
  • https://github.com/kubernetes-sigs/kubebuilder/issues/3402#issuecomment-2122171734

camilamacedo86 avatar May 21 '24 09:05 camilamacedo86