[Umbrella issue] Scaffolding enhancements

[Adirio]

Description

The following issue will group together all the different enhancements aimed to obtain a smoother file scaffolding system that improves redability, removes duplicate sections, avoids calling several methods multiple times, and centralizes functionality.

Some of the enhancements will be straight forward and will be directly implemented by a PR while others will have their own issue in order to contain the discussion. Please, keep comments in this issue just related to general topics related this issue or suggestions on which other enhancements to apply and use the respective issues or PR to talk about each of the enhancements.

Motivation

The main scheme followed by kubebuilder is the following:

1. Create a Cobra command and subcommands
2. This command creates a scaffolder for the specific command that:
   1. Sets default values
   2. Validates the provided input
   3. Creates the specific files

Issues found

• Some commands do not have an associated scaffolder and their scaffolding logic is done in the command itself (see cmd/webhook_1.go or cmd/webhook_2.go as examples).
• Validations are spread all accross the project. Some of them are done by the command and some by the associated scaffolder.
• Files are scaffolded with multiple non-needed fields (for example, ProjectPath is not required for any file and is provided to all, in pkg/scaffold/v1/crd/doc.go a field called Comments is not used, ...), information about the resource is kept outside the resource (GroupDomain, ResourcePackage), or stored in two places (Resource.Resource vs Plural).
• Resource is being validated by the scaffolders and by each file. This implies that, for example, for a single kubebuilder create api ... command the resource is validated 13 times.

Proposed solutions:

• Scaffolders for commands that do not have one should be created to keep the scaffolding logic in pkg/scaffold and not in cmd.
• Validation, unless directly related to the command, should be delegated to the scaffolder.
• Design interfaces for both scaffolders and files.
• Re-design the Resource struct to contain the fields that are outside: GroupDomain, Plural and ResourcePackage.
  • Determine if both Resource.Resource and (Resource.)Plural are needed or keep only one of them.

Related Issues and Pull Requests

Issues:

• [Scaffold enhancement] File interface #1226
• Split scaffold package #1264
• Template mixins #1358

Pull requests:

• Remove scaffolding fields that were not used #1219
• Move files inside a new directory #1221
• Add interface validation check for missing types #1248
• Named arguments for path formatting #1252
• Two-step resource creation #1255
• Split injecting fields and validating files #1265
• Validate resources for webhooks and tests #1266
• Include PROJECT file information into model.Universe #1313
• Offer a flag to change the image name #1314
• Fix unofficial way of handling already existing files #1316
• Move configuration file related types and functions to separate package #1325
• 🏃 Homogenize commands through Scaffolder interface #1330
• Move scaffold machinery to its own internal package #1339
• Move file related models to pkg/model/file #1340
• Group templates under a internal subpackage #1348
• Implement Wrapper interface for errors #1349
• Template mixins #1359
• Update files as part of the Scaffold.Execute #1375

More coming soon

/kind feature

[DirectXMan12]

awesome! thanks for working on this :-)

[DirectXMan12]

So, these are the names that exist according to CRDs:

• Resource Singular (.spec.names.singular, optional): singular resource name: prometheus. Defaults to lowercase kind in the CRD object.
• Resource Plural (.spec.names.plural): plural resource name: prometheis (Prometheus has weird pluralization rules)
• Resource Kind: (.spec.names.kind): Prometheus

So, we should have an internal representation of each of those. Without digging further, it's not clear what the different between Resource and Plural is supposed to be, but if I had to guess, it may have come from that distinction. At any rate, Resource.Resource should at least be renamed.

[Adirio]

Right now, pkg/scaffold includes very different concepts in a confussing way.

• Models: e.g., pkg/scaffold/resource subpackage is modelling resources.
• Configuration: spread all over the package, type defined in pkg/scaffold/input, methods defined in the middle of files and even duplicated (logic to save the configuration file is both in pkg/scaffold/scaffold.go and in cmd/edit.go packages).
• Scaffolders: pkg/scaffold/api.go and pkg/scaffold/project.go. The rest of the commands implement the scaffolder logic directly.
• Scaffolding machinery: pkg/scaffold/scaffold.go, pkg/scaffold/output.go, pkg/scaffold/input and pkg/scaffold/scaffoldtest.
• File templates: pkg/scaffold/project, pkg/scaffold/v1 and pkg/scaffold/v2.

The plan is splitting these different parts into separate packages or at least subpackages.

• Models: a first PR into this direction was made by Initial addons via plugin model #943 including the model package. A small extension of this package has been pushed in Include PROJECT file information into model.Universe #1313. Resource should also be placed here merging model.Resource and resource.Resource into a single type. But first an enhanced version of the pkg/scaffold/resource package is proposed in Two-step resource creation #1255. models package name seems to be a better fit than the singular form, and subpackages may be needed as the package grows.
• Configuration: Move configuration file related types and functions to separate package #1325 moves all the configuration related logic to internal/config and model/config.
• Scaffolders: some commands, such as webhook (both versions) or edit implement the scaffolding logic directly. This logic should be extracted to their own scaffoldings and a common interface for scaffolders should be defined.
• Templating machinery: the underlying templating machinery should only be accessed though scaffolders so moving it to pkg/scaffold/internal/machinery sounds reasonable. Previous point is required.
• File templates: all file templates should be grouped. pkg/scaffold/internal/templates is the option I like most. Extracting it out of the pkg/scaffold package doesn't make that much sense and making it internal seems reasonable as only the scaffold should access the templates.

The resulting structure would be:

+ cmd: will contain only the command and flags related logic
|   + ...
+ internal
|   + config
+ pkg
|   + model
|   |   + config
|   |   + file
|   |   + resource
|   |   + universe
|   + scaffold
|   |   + internal
|   |   |   + machinery
|   |   |   + templates
|   |   |   |   + v1
|   |   |   |   |   + ...
|   |   |   |   + v2
|   |   |   |   |   + ...

@DirectXMan12 @mengqiy I would apreciate some feedback on this topic. Also, #1255, #1313 and #1325 need some love as they are blocking further progress in the scaffolding enhancement.

[Adirio]

I made a internal package dependency schema:

• It considers the state after 🏃 Homogenize commands through Scaffolder interface #1330 gets merged.
• Packages pkg/scaffold/v1 and pkg/scaffold/v2 have been considered to be flat packages (no subpackages) to simplify the number of arrows.
• Dashed arrows mean that there is a test package (same name but with the _test suffix) at that path that has that import, but they do not incur in import loops, as test packages can't be imported.
• Packages have been grouped in 5 different categories:
  • CLI
  • Modelling
  • Scaffold
  • Templates
  • Plugins

• pkg/scaffold/input, pkg/scaffold/resource, and pkg/scaffold/util, despite being subpackages of pck/scaffold fit better in the modelling category.
• "CLI" is not imported from outside, which is desired.
• "Plugins" is only imported from "CLI".
• "Scaffold" is only imported from "CLI" and test packages of "Templates".
• "Templates" are only imported from "Scaffold".
• "Modelling" is imported from all of them.

I think this schema makes a lot of sense.

In order to achieve this, the three packages mentioned above still have to be moved into the "modelling" category.

• Two-step resource creation #1255 moves pkg/scaffold/resource and pkg/scaffold/util to pkg/model/resource
• pkg/scaffold/input has no PR moving it yet.

After #1255 gets merged, the schema would look like:

[craigtracey]

@DirectXMan12 with regard to pluralization, I agree on "weird pluralization rules." For instance, "Repo" would be another example. Currently that will pluralize as "Repoes."

Apart from these examples, the pluralization is currently based on English rules. It is certainly conceivable that users will have resource types that are not English words.

I have taken a look, and this particular change will percolate through both kubebuilder and controller-gen.

[Adirio]

There are probably some other enhancements that can be done but the main scaffold refactoring has already been merged.