✨re-scaffold the project with a new command

[yyy1000]

This is the first part of the whole project:
only the alpha command and be able to re-scaffold the init command for example for the layout.

The demo video is here:

first-PR-video-show.mp4

[k8s-ci-robot]

Hi @yyy1000. 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.

[camilamacedo86]

WDYT about add a long description here?

Maybe we can describe that this command is an experimental feature that has the purpose of ....

[camilamacedo86]

What about we add the Examples, such as:

kubebuilder/pkg/cli/completion.go

Lines 49 to 57 in db9def4

	Example: fmt.Sprintf(`# If shell completion is not already enabled in your environment you will need
	# to enable it. You can execute the following once:
	$ echo "autoload -U compinit; compinit" >> ~/.zshrc
	# To load completions for each session, execute once:
	$ %[1]s completion zsh > "${fpath[1]}/_%[1]s"
	# You will need to start a new shell for this setup to take effect.
	`, c.commandName),

Just a couple of them based on what it is doing now.

[yyy1000]

OK, I will follow that!

[camilamacedo86]

Another place where you can check an example over how to do that is here: https://github.com/operator-framework/operator-sdk/blob/5347d9375658cab9117e7ac8f691d35bb9154ff9/internal/cmd/operator-sdk/generate/bundle/bundle.go#L70-L109

[camilamacedo86]

Could we encapsulate this code in a func like run() since it will probably grow?

[yyy1000]

Good suggestion! I will do that.

[camilamacedo86]

I think we need a func like setDefaults which will set the defaults (here you can find a nice example)

Also, one validate() which we should call before do anything and has the purpose to validate if ALL is OK prior run the command or fail. What do you think?

[yyy1000]

Cool! I used to have a validate() in it, but I don't know what it should check so I delete it. Maybe it can check 'whether it can find the kubebuilder binary'. :)

[camilamacedo86]

I think you must ensure that has a PROJECT file
Otherwise, how will you do the re-scaffold?

For now, I think just that. But we will need also do checks like
If the Project file has layout == go/v2 or go/v3 AND if the Project file is the old config version than we say that the command is not supported. but we can do that in a follow up.

[yyy1000]

Thanks! So far my code will ensure the PROJECT file.
I will add other check in a follow up. :)

[camilamacedo86]

I think we should either have a GenerateCMD struct so that we can define and store all values that will be used by it. Then, the funcs will have it as a receiver like (GenerateCMD cmd) run() or (GenerateCMD cmd) validate() and etc.

WDYT?

[yyy1000]

Great! I will try to change the code to make it. :)

[camilamacedo86]

You made a terrific work 🥇
I just suggested a couple of nits that are more related to improvements to help you grow the command in your journey.

Anyway, that is amazing work !!!!

[camilamacedo86]

/ok-to-test

[varshaprasad96]

If we encounter errors here, do we want to proceed with the next steps? If not, we can exit with os.Exit(1).

[yyy1000]

Good idea!

[varshaprasad96]

Do we need this? Looks like after doing the initial scaffolding we are not taking any actions, so we can need not necessarily set the context to where we started.

[varshaprasad96]

Do we also want to check if the input and output directory exist with os.Stat? Are we doing it somewhere?

[yyy1000]

Yes, we can check them. There're some places using it.

[varshaprasad96]

So we could store the input values in the struct and manipulate it separately. For example, we could have:

type migrateOptions struct{
    inputDir string
    outputDir string
}

Then you can bind the values from these flags to the fields in the struct. For example:

opts := &migrateOptions{}

cmd.Flags().StringVar(&opts.inputDir, "input-dir", "<>" , "<>")

And then you can have methods on this struct to execute the code.
like:

func (opts *migrateOptions) Complete() error {
    // logic for checking input and output dir etc
}

[yyy1000]

I will try it. :)

[camilamacedo86]

Suggested change

	$ ls my-output/
	Dockerfile Makefile PROJECT README.md cmd config go.mod go.sum hack

I think we do not need to put an example of the output.

[yyy1000]

Solved it and below. :)

[camilamacedo86]

Suggested change

	# make sure the PROJECT file is in the 'input-dir' argument, the default is the current directory.
	# make sure the PROJECT file is in the 'input-dir' argument, the default is the current directory.

[camilamacedo86]

Great work 🥇

I will let @Kavinjsir and @varshaprasad96 give the second review
If they be OK with they will lgtm and this one will get merged

[camilamacedo86]

@yyy1000

Before merging we need to squash the commits. Could you please do that?

/approved cancel

[camilamacedo86]

@yyy1000

Before merging we need to squash the commits. Could you please do that?

/approved cancel

[camilamacedo86]

@yyy1000

Before merging we need to squash the commits. Could you please do that?

/approved cancel

[Kavinjsir]

Left some comments for reference. Other than that, the PR shows great 💯

[Kavinjsir]

Nits: Update 2022 -> 2023.

May probably check other updated files also

[Kavinjsir]

nit:
kuberbuilder -> Kubebuilder

[Kavinjsir]

Question:
Do we need to give that much permission as ModePerm to the folder?

[yyy1000]

Oh, that's a question. What about setting it to 0755(the default permission of directory)? According to https://www.redhat.com/sysadmin/introduction-chmod

[Kavinjsir]

I am cool with 0755.

OR maybe we can refer to the default file permissions set by internal scaffold package:
https://github.com/kubernetes-sigs/kubebuilder/blob/v3.10.0/pkg/machinery/scaffold.go#LL38-L39 :

	defaultDirectoryPermission os.FileMode = 0700
	defaultFilePermission      os.FileMode = 0600

[Kavinjsir]

Well, for a second thought, I'd prefer 0755:

1. This makes the permission of the root folder path more general. (Should be similar as other directories in users' hosts)
2. The permissions of the internal files/directories can be handled by the scaffolding package which will be regulated by these values (e.g: defaultDirectoryPermission).

cc: @camilamacedo86 @varshaprasad96 Any suggestions on this?

[Kavinjsir]

As discussed, we can apply 0755.

[Kavinjsir]

In my own opinion:
It is better to have a consistent behavior when handling the incoming parameter outputPath.
We can spell an absolute path when defining a DefaultOutputDir.

---

As for this getOutputPath function, should this functionality to be:

1. check if given outputPath already exists; if yes, then return err
2. check if given outputPath is an absolute path; if yes, then return the value
3. if given outputPath is a relative one, concatenate with file path prefix (making it an absolute one), then return?

---

Just some thoughts for reference
cc: @camilamacedo86 @varshaprasad96

[yyy1000]

Sounds good to me. Let's see whether others have different views.

[varshaprasad96]

I'm curious on the necessity to do the comparison. The logic in my mind should be:

1. Check if output dir is provided. If not default it to pwd+DefaultDir
2. If not, do an os.Stat to check if the output dir already exists.
3. If exists, err. If not, create a new one.

[Kavinjsir]

For readability, can we avoid having abbreviations for parameters?

Like cwd -> currentWorkingDirectory ?

[Kavinjsir]

Is this ready in use OR we will have future development for this?
(In other words, do we need to add a todo comment here?)

[Kavinjsir]

Also, will we call Kubebuilder executable to re-scaffold the project in the future? It feels hacky to me...

[yyy1000]

Yes, this is in use. We will call that to init the project from 'PROJECT file'

[Kavinjsir]

What about naming the variable as config to help understand the context?

[Kavinjsir]

lgtm

[varshaprasad96]

/lgtm

Nice work @yyy1000!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Kavinjsir, varshaprasad96, yyy1000

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:

• OWNERS [camilamacedo86,varshaprasad96]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[camilamacedo86]

/lgtm

[camilamacedo86]

@yyy1000 this lint issue we can ignore in this case
Add a comment here

Suggested change

	if err := os.MkdirAll(opts.OutputDir, 0755); err != nil {
	// nolint: gosec
	if err := os.MkdirAll(opts.OutputDir, 0755); err != nil {

So that we will ignore this specific check in this specfic line then we can get this one merged

[yyy1000]

added and squashed.

[camilamacedo86]

/lgtm

[camilamacedo86]

Hi @yyy1000,

See the lint error:

run golangci-lint
  Running [/home/runner/golangci-lint-1.51.2-linux-amd64/golangci-lint run --out-format=github-actions --new-from-patch=/tmp/tmp-2040-jjQ4M5ZBUurK/pull.patch --new=false --new-from-rev=] in [] ...
  Error: File is not `gofmt`-ed with `-s` (gofmt)
  
  Error: issues found
  Ran golangci-lint in [26](https://github.com/kubernetes-sigs/kubebuilder/actions/runs/5228672632/jobs/9441403653?pr=3431#step:5:28)06ms

To fix this one run make lint-fix. This issue will be fixed automatically with this command, then you can commit squash and push for we get this one merged.

You can either to do this test locally by running make lint.

[yyy1000]

Hi @yyy1000,

See the lint error:

run golangci-lint
  Running [/home/runner/golangci-lint-1.51.2-linux-amd64/golangci-lint run --out-format=github-actions --new-from-patch=/tmp/tmp-2040-jjQ4M5ZBUurK/pull.patch --new=false --new-from-rev=] in [] ...
  Error: File is not `gofmt`-ed with `-s` (gofmt)
  
  Error: issues found
  Ran golangci-lint in [26](https://github.com/kubernetes-sigs/kubebuilder/actions/runs/5228672632/jobs/9441403653?pr=3431#step:5:28)06ms

To fix this one run make lint-fix. This issue will be fixed automatically with this command, then you can commit squash and push for we get this one merged.

You can either to do this test locally by running make lint.

Oh, thanks!
I forget to run make lint to check this. Fixed it already.

[camilamacedo86]

/test pull-kubebuilder-e2e-k8s-1-27-1

[camilamacedo86]

/lgtm