Project file with all user input data

[camilamacedo86]

Description

Ensure that we are storing all input data in the PROJECT file.

Motivation
I am maintainer like to ensure that we have stored all input data used to scaffold the projects by the user so that I could propose a subcommand/plugin to help users migrate from a plugin version X to X+1 since would be possible scaffold the project from the scratch. Also, it might be helpful in many scenarios to tooling the scaffolds.

Mor info: See: #1644 (comment)

What is missing to be stored

• Webhooks data
• resource/controller=[true|false]

/kind feature

[camilamacedo86]

Hi @estroz, @joelanford, @DirectXMan12, @pwittrock I am pushing it for v3+plugin for we have the chance to discuss it and try to achieve it for the release if we agree on that. Looking for your inputs as well.

[estroz]

hasResource and hasController for the resources

Maybe only hasResource? If you're building a type library you would set --controller=false --resource=true, so we'd probably want to set hasResource: true or types: true field for its GVK. Conversely, one major reason you would have --resource=false --controller=<true|false> set is if you're using an external type; the config can store an external type path as something like external: <import path> (see #1772) instead of controller: true.

from the init owner and license

These are already captured in hack/boilerplate.go.txt so these don't need to be tracked in the config.

[camilamacedo86]

Hi @estroz,

These are already captured in hack/boilerplate.go.txt so these don't need to be tracked in the config. 👍

Totally agree. Good point.

Maybe only hasResource?

Currently, the GVK is only persistent on the PROJECT file if the resource is true. So, how the allow all combinations shows that the best way is we store both booleans as use the api one to check if the api was or not scaffolded. See my suggested solution #1849. PS.: I think it will also be beneficial for we address the scenarios raised/discussed in #1772. I hope that you liked it.

[estroz]

Prior to releasing v3.0.0, the config spec for version 3-alpha needs to be stabilized. A few changes have been added to or are proposed for 3-alpha. To summarize:

• Added resources[].crdVersion and resources[].webhookVersion when --crd-version and --webhook-version are set, respectively (⚠️ (go/v3-alpha) default to v1 CRDs and webhooks #1644)
• Add resources[].api and resources[].controller when --resource and --controller are set, respectively (✨ (go/v3-alpha) store data info to know if the controller and/or api were scaffolded #1849)
• Add resources[].webhooks for defaulting, validating, and conversion webhooks (✨ store all config info (go/v3-alpha)  #1696)

These additions are being made (not intentionally) without considering what they'll look like together, but should be. Perhaps a redesign of resources is in order instead of tacking on new fields to the version 2 implementation. Something like the following:

version: 3-alpha
resources:
- api: # api contains all important information from `create api`
    group: my.domain
    version: v1
    kind: CronJob
    crdVersion: v1
    resourceTypes: true # If 'api/v1/cronjob_types.go' was created
    controller: true # If 'controllers/cronjob_controller.go' was created
  webhook: # webhook contains all important information from `create webhook`
    webhookConfigVersion: v1
    types:
    - validating # If `webhook.Validator` was implemented
    - mutating # If `webhook.Defaulter` was implemented
    - conversion # If `--conversion` was set

The above design groups fields by create subcommand, which is logical given the expected kubebuilder command flow of init -> create api -> create webhook.

@Adirio @DirectXMan12 @pwittrock thoughts on making a drastic design change prior to the v3.0.0 stable release?

[Adirio]

My thoughts but written by you. 100% agree. And 100% in favor of omiting as many of these fields as possible if they are default values.

[joelanford]

Each plugin can define its own flags, so would the plugin also control these fields? If so, that kind of points to these fields being stored under the plugin configuration somewhere, possibly.

Either way, I feel like this is a pretty major change right as we're trying to stabilize v3. I would vote we hold this out of v3 and make it a candidate for v4.

[estroz]

Each plugin can define its own flags, so would the plugin also control these fields?

Yes. These fields are intended to be general enough such that any plugin can make use of them. If they aren't used by a plugin, then they're omitted from the config, so the user will never have to worry about them.

Either way, I feel like this is a pretty major change right as we're trying to stabilize v3

I am worried about that. Perhaps we should punt on this issue, #1696, and #1849 for now and go with what exists in the current spec, since none of the fields mentioned in this issue or those PRs are necessary for v3.0.0 features.

[Adirio]

What do you think about the following model:

version: 3-alpha
domain: my-domain.com
crdVersion: v1beta1
webhookVersion: v1beta1
apis:
- resource:
    group: Ships
    # version omitted means v1
    kind: Boat
    # plural omitted means Boats
    # domain omitted means my-domain.com (see line 2)
    # path omitted means it is not an external type
  definition:
    # version omitted means v1beta1 (see line 3)
    namespaced: true
  # controller omitted means false
  webhooks:
    # version omitted means v1beta1 (see line 4)
    types:
    - validating
- resource:
    group: Animals
    version: v2
    kind: Sheep
    plural: Flock
    domain: my-animals.com
    path: "github.com/owner/repo/apis/animals"
  controller: true

Templates allow methods that return a single value to be used in templates as if they were fields: {{ .Resource.ImportAlias }}. You can also provide methods with 2 output arguments if the second one is an error, in which case if the method returns an error it will be returned by template.Execute function. We can use them for ImportAlias and GroupPackageName that don't need to be saved in the project config file as they are computed from other fields but they need to be provided to the templates.

[camilamacedo86]

Hi @Adirio and @estroz,

Thank you for your contribution.

I do NOT agree with them because controllers are NOT APIs and what is suggested here is out of the scope of this issue. Also, see that the proposals here are ONLY to start to track the missing info and NOT change the PROJECT model design at all.

I totally agree with @joelanford that we should NOT redesign the PROJECT file for v3. However, folks feel free to raise new issues with your ideas with its purpose to be addressed for v4 plugin. Why we should get merged the (Webhooks data: PR: #1696 and resource/controller=[true|false] #1849) as it is shaped so far for v3?

• It will allow us to provide a tool to migrate in the future project from v3 for v4 plugin whcih will address a common request made by the community. Users have been reporting that they find hard this process indeed in the SDK survey
• It will allow us to improve the tooling features for the plugins.
• It will increase the maintainability
• The PRs are not changing the current design. The PRs are only adding the respective fields as we have been discussing for a while such as we start to track the crdVersion and the webhookVersion. Then, if we decide to change its design in the v4 as you are suggesting we can tooling the migration and make this process easier for the users.

[camilamacedo86]

Hi @Adirio and @estroz,

Currently, we have the following model:

domain: testproject.org
layout: go.kubebuilder.io/v3-alpha
multigroup: true
projectName: project-v3-multigroup
repo: sigs.k8s.io/kubebuilder/testdata/project-v3-multigroup
resources:
- crdVersion: v1
  group: crew
  kind: Captain
  version: v1
  webhookVersion: v1

After we merge the PRs #1696 and #1849 we will have the model:

domain: testproject.org
layout: go.kubebuilder.io/v3-alpha
multigroup: true
projectName: project-v3-multigroup
repo: sigs.k8s.io/kubebuilder/testdata/project-v3-multigroup
resources:
- crdVersion: v1
  group: crew
  kind: Captain
  version: v1
  webhookVersion: v1
  api:true // is true when a GKV has an API scaffolded
  controller:false   // is true when a GKV has an controller scaffolded
  webhooks: // respresents the webhooks
    defaulting: true
    validation: true

Why I do not agree with the suggestions made #1826 (comment)?

#1772 (comment)

Regards the suggestion made in #1826 (comment):

It shows very closely as it is now.

• @estroz I am ok with we move the webhook version to webhooks 👍

webhook: # webhook contains all important information from `create webhook`
    webhookConfigVersion: v1

• I am afraid that I do not agree with changing the attrs names to singular or rename to matches with the subcommands names since it does not reflects its real structure/objects or better what actually its represents in the project. It shows hurts concepts such as DDD. For example; the controller is not api, 1 GVK we can have N webhooks scaffolded it is an [], the command is in the singular because does 1 scaffold per execution, however, the model has 0..N ( Many ). So, I hope we are able to agree to keep the names as they are now.

• The motivations for the following was just not scaffolded webhooks: {} Am I right? It is solved by applying the @Adirio suggestion already. So, I understand that it is no longer required.

types:
    - validating # If `webhook.Validator` was implemented
    - mutating # If `webhook.Defaulter` was implemented
    - conversion # If `--conversion` was set

• Why the nomenclature resourceTypes: true # If 'api/v1/cronjob_types.go' was created is more appropriate/intuitive than api:true to represents that an API was scaffolded? What means resourceTypes? Is really only the _types.go the file that is generated which represents the api created to the project? I do not think so, so I am open for new names and suggestions for we address it in ✨ (go/v3-alpha) store data info to know if the controller and/or api were scaffolded #1849, however, resourceTypes shows not feel right yet.

Suggestion/Question

So, besides I still believing that re-design the PROJECT file is out of the scope of this issue, if we still looking for doing this changes here and if my and if my above comments did not help us to reach a consensus already then, WDYT about tries to list what should change and why? It might be helpful to add use cases to illustrate it as what are the problems faced with the current or suggested changes in the model.

[Adirio]

resources[].webhooks.webhookConfigVersion is too verbose, resources[].webhooks.version should be more than enough.

I wouldn't use subcommand names either. Subcommands are based on the most common workflows followed by projects. The most commong thing is to scaffold both the resource type and its controller and that is why they are grouped. Webhooks are less common so they are provided as a separate subcommand. However I do think we need to group and structure this field better.

Previously, the project file just stored GVK (Group-Version-Kind). Having two object that represent a resource for the config file (pkg/model/config.GVK) and for internal use (pkg/model/resource.Resource) wasn't that much of an issue mainly because pkg/model/config.GVK was pretty small and its a k/k concept. However we have been adding fields and more fields to this pkg/model/config.GVK to the point where it has transitioned into pkg/model/config.ResourceData. Which is the problem with this? we have two different types that represent the same concept for the project configuration file and internal usage. This is proof of a poor design, and will arise a lot of problems in the future, for example when working with out-of-tree plugins. So, I strongly think that we should have a single Resource model that is able to store all this information and save it to the config file as needed.

I think that another issue here is that each of us is calling API to a different thing. You are calling API to whatever gets scaffolded when you do kubebuilder create api --resource --controller=false ... which in reality is just the golang type definition and the CRD, and that is the reason why I used the term Definition in my model to refer to it. The controller, from a kubebuilder viewpoint, is also part of the API as you create it with kubebuilder create api --controller .... Webhooks, however, are not created by kubebuilder create api despite being scaffolded in the api[s] directory. Probably this is due to the fact that webhhoks are not required as often as the controller and because in v1 webhooks weren't supported from the start and they were added as a kubebuilder alpha subcommand. The point here is that API is missleading and that's the reason why I would not use it. I suggest using Definition, Controller and Webhooks to refer to the three parts that we are scaffolding. I think that stop using the term API will make things clear (I'm not suggesting to remove the command name api, at least for backwards compatibility reasons)

[camilamacedo86]

Hi @estroz and @Adirio,

To make it easier I centralized all changes in the same PR: https://github.com/kubernetes-sigs/kubebuilder/pull/1696/files. Also, I addressed all suggestions that shows that we 3 can reach a consensus:

componentConfig: true
domain: testproject.org
layout: go.kubebuilder.io/v3-alpha
projectName: project-v3-config
repo: sigs.k8s.io/kubebuilder/testdata/project-v3-config
resources:
- api: 
     namespaced: true  
     crdVersion: v1
  controller: true
  group: crew
  kind: Captain
  version: v1
  webhooks:
    apiVersion: v1
    defaulting: true
    validation: true