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

Add support for configuring boilerplatePath during API scaffolding on plugin base.go.kubebuilder.io/v4 #3697

Closed
ardikabs opened this issue Nov 20, 2023 · 14 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@ardikabs
Copy link

ardikabs commented Nov 20, 2023

What do you want to happen?

Description:

Currently, I've configured my operator to adhere to an opinionated directory structure preference, causing me to execute kubebuilder with a non-default plugin, which is base.go.kubebuilder.io/v4. However, kubebuilder seems to define a hardcoded boilerplate path attribute in hack/boilerplate.go.txt during API scaffolding, as can be seen below at line 70:

func (s *apiScaffolder) Scaffold() error {
log.Println("Writing scaffold for you to edit...")
// Load the boilerplate
boilerplate, err := afero.ReadFile(s.fs.FS, hack.DefaultBoilerplatePath)
if err != nil {
if errors.Is(err, afero.ErrFileNotFound) {
boilerplate = []byte("")
} else {
return fmt.Errorf("error scaffolding API/controller: unable to load boilerplate: %w", err)
}
}

Feature Request:

Could it be configured to accommodate an opinionated directory structure when attempting to satisfy certain preferences? say i expect the boilerplate refer to tools/hack/boilerplate.go.txt, as compared to controller-gen tool that could refer to a custom headerFile like below:

controller-gen object:headerFile="/path/to/boilerplate.go.txt"`

Plugin: base.go.kubebuilder.io/v4

Extra Labels

No response

@ardikabs ardikabs added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 20, 2023
@ardikabs ardikabs changed the title Support configuring boilerplatePath during api creation on plugin base.go.kubebuilder.io/v4 Support configuring boilerplatePath during API scaffolding on plugin base.go.kubebuilder.io/v4 Nov 20, 2023
@ardikabs ardikabs changed the title Support configuring boilerplatePath during API scaffolding on plugin base.go.kubebuilder.io/v4 Add support for configuring boilerplatePath during API scaffolding on plugin base.go.kubebuilder.io/v4 Nov 20, 2023
@camilamacedo86
Copy link
Member

camilamacedo86 commented Nov 23, 2023

Hi @ardikabs,

Thank you for bringing up this issue. I agree that the ability to customize all paths, not just specific ones, would be a valuable addition to Kubebuilder. A config file allowing users to define paths for controllers, hack, etc., would indeed enhance the tool's flexibility.

However, introducing such customization for only one aspect without addressing the others would lead to inconsistency in behavior. We aim for a uniform and predictable user experience. Therefore, implementing this feature across the board is crucial, albeit it introduces significant complexities.

IHMO: Given these considerations, the best approach would be to develop a detailed proposal. This document would outline the implementation strategy, consider potential problems, and propose solutions. You can find a template for proposals here: https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/template.md, and some examples at: https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs.

As a temporary measure for the current issue, one possibility could be to issue a warning instead of an error when the boilerplate is not found, though this might be perceived as a bug by some users. However, it might be an acceptable interim solution.

We're definitely open to contributions that would help us move in this direction.
Would you be interested in working on this feature?

@ardikabs
Copy link
Author

ardikabs commented Nov 29, 2023

Hi @camilamacedo86 , seems I missed the notification, sorry for the late response.

It seems there is no error returned when hack/boilerplate.go.txt file is missing because it will be overridden with an empty value, the only error that occurs is when hack/boilerplate.go.txt isn't a valid file like permission denied on read, etc.

Nevertheless, I am willing to address this concern by modifying kubebuilder to issue a warning rather than an error when an issue arises while reading the file.

Regarding the design proposal, I am interested in contributing to it. However, I'm unsure if focusing solely on hack/boilerplate.go.txt is acceptable as a design proposal, as it appears to be a relatively minor feature request.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Dec 5, 2023

Hi @ardikabs,

Regarding the design proposal, I am interested in contributing to it. However, I'm unsure if focusing solely on hack/boilerplate.go.txt is acceptable as a design proposal, as it appears to be a relatively minor feature request.

That is great, and you are right. The proposal should be a solution that provides a design that allows users to customise the path for controllers and APIs as well. We cannot allow it partially. Otherwise, the UX will be hard. However, I am proposing a third option that might be acceptable.

Nevertheless, I am willing to address this concern by modifying kubebuilder to issue a warning rather than an error when an issue arises while reading the file.

I was thinking better about this one.
If we add a warning, what value does that bring?
Users still need to be able to have the header scaffolded.

So I think we can:

a) Improve the error message, say that the file was not found, and outline the Options. use None if you do not want any scaffold

b) An alternative option might be:
Add a new flag to the kubebuilder edit --bolerplatPath
Then, add a new spec to the PROJECT file to store the bollerplatPath
Change the tool to use the path in the Project File instead

would you like to work on this one?

@mateusoliveira43
Copy link
Contributor

About current structure (using go/v4 and kubebuilder 3), is there any motivation to separate go files in 3 folders (api, internal and cmd). Adding them under a single folder could be a good approach? Example

├── pkg (or internal)
│   └── cmd
│       └── main.go (or without a sub folder, directly in pkg level)
│   └── controller
│   └── api

@camilamacedo86
Copy link
Member

camilamacedo86 commented Dec 15, 2023

Hi mateusoliveira43,

is there any motivation to separate go files in 3 folders (api, internal and cmd).

Yes, the default layout in Kubebuilder adheres to the standards and best practices established by Golang and Kubernetes communities. Here's a brief explanation of the directory structure:

  • api: This directory contains the Go files for APIs. It's where the API types and associated CRD definitions are located.
  • internal: This directory is intended for code that shouldn't be imported by other applications. In your case, it would typically include the controllers.
  • cmd: This is where the command-line interfaces are defined. For instance, initialization and manager code are placed here.
  • hack: The reason why the license file is located in the hack directory is because it follows a Kubernetes convention. The hack directory typically contains scripts that are useful for development, such as those for building, testing, and debugging the project.

For more information, you can refer to these resources:

Please note that any changes in the project layout and the scaffolding process are extensively discussed within the community and are not made lightly.

Furthermore: https://book.kubebuilder.io/migration/legacy/v2vsv3#project-customizations

Screenshot 2023-12-15 at 17 38 02

@mateusoliveira43
Copy link
Contributor

thanks for the detailed explanation @camilamacedo86 !

@ardikabs
Copy link
Author

ardikabs commented Dec 21, 2023

Hi @camilamacedo86 , i have submitted a PR for this request, but instead of storing to the PROJECT file, it seems to me that would be better to loosen that constraint and allow adjustment through flags on every attempt when invoking the command (e.g., init, create api, and create webhook)

@camilamacedo86
Copy link
Member

Hi @ardikabs

Hi @camilamacedo86, i have submitted a #3716 for this request, but instead of storing to the PROJECT file, it seems to me that it would be better to loosen that constraint and allow adjustment through flags on every attempt when invoking the command (e.g., init, create API, and create webhook)

What you want to change here is related to the base of the Project
That means the only way to do that would be in the init command
Then, could you track it out in the ProjectConfig as described in the PR?

Change it is outside of the domain of responsibility of create API, and create webhook commands

@ardikabs
Copy link
Author

ardikabs commented Feb 6, 2024

I see, in other words, we need to introduce a new field in the PROJECT file, yes?
And, it seems to me that this would also make sense if we add a possibility to change the boilerplate path under the edit command, what do you think? @camilamacedo86

@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 7, 2024

Hi @ardikabs

Yes. We need to:

  • a) add the new spec to the ProjectConfig, i.e. header-file-path or boilerplate-path. It is important not to break existing projects if the value is not found in the Config; we set the default path. (see the pkg/config:
    type Cfg struct {
    // Version
    Version config.Version `json:"version"`
    // String fields
    Domain string `json:"domain,omitempty"`
    Repository string `json:"repo,omitempty"`
    Name string `json:"projectName,omitempty"`
    PluginChain stringSlice `json:"layout,omitempty"`
    // Boolean fields
    MultiGroup bool `json:"multigroup,omitempty"`
    ComponentConfig bool `json:"componentConfig,omitempty"`
    // Resources
    Resources []resource.Resource `json:"resources,omitempty"`
    // Plugins
    Plugins pluginConfigs `json:"plugins,omitempty"`
    }
    )
  • b) allow passing the flag in the init|edit commands
  • c) ensure that the hack/boilerplate.go.txt will be generated in this path
  • e) that in this case, hack/boilerplate.go will not exist/delete (if has only this file in the hack, delete the dir; otherwise, the file)
  • f) also, the makefile target will be scaffolded or updated accordingly ( see an example of how we do those replaces in :
    if err := util.ReplaceInFile(
    controller.Path,
    "//TODO: scaffold container",
    fmt.Sprintf(containerTemplate, // value for the image
    strings.ToLower(s.resource.Kind), // value for the name of the container
    ),
    ). If it is not possible to update, then we should warn

Lastly, we need an e2e test to ensure the functionality.
Ideally, we would need to ensure the init/edit command, like described in: #3716 (comment)

I hope that helps clear things up. Thank you a lot for looking on that.

@k8s-triage-robot
Copy link

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

This bot triages un-triaged issues 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 as fresh with /remove-lifecycle stale
  • Close this issue 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 May 7, 2024
@k8s-triage-robot
Copy link

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

This bot triages un-triaged issues 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 as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

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

/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 6, 2024
@k8s-triage-robot
Copy link

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

This bot triages issues 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:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

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

/close not-planned

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 6, 2024
@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

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

This bot triages issues 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:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

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

/close not-planned

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

5 participants