🐛: (go/v4) Scaffold no header when lincense is type none #3391

yyy1000 · 2023-05-06T11:28:03Z

Description
Fix #3377
Now it would not scaffold any header if we use 'none' in license option.

k8s-ci-robot · 2023-05-06T11:28:12Z

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 · 2023-05-06T15:29:34Z

hack/docs/generate_samples.go

@@ -51,7 +50,7 @@ func UpdateComponentConfigTutorial() {
 }

 func UpdateCronjobTutorial() {
-	binaryPath := util.KubebuilderBinName
+	binaryPath := "/tmp/kubebuilder/bin/kubebuilder"


Could we revert this one and still use the const?

I think use the const 'util.KubebuilderBinName' couldn't work. This will not use the kubebuilder binary complied by the command. 🤔 See #3390

It has been working so why it would stop to work within the changes of this PR?
Can you please revert this one and squash for we get it merged?

well, I think it has been working for only two days. The last commit changing it is : d0a1806
So I think at least not so many people run it.

This code is called when we run make generate.
The make generate is called when we do the CI checks and would NOT get merged if does not work.

See this code successfully executed to verify your PR: https://github.com/kubernetes-sigs/kubebuilder/actions/runs/4902477788/jobs/8754346010

I can ensure that works either and you can properly very that by just running without these changes make generate locally within the master branch.

Could you please revert these changes that have no relation to the scope of the PR?

camilamacedo86 · 2023-05-06T15:29:44Z

hack/docs/generate_samples.go

@@ -36,7 +35,7 @@ func main() {
 }

 func UpdateComponentConfigTutorial() {
-	binaryPath := util.KubebuilderBinName
+	binaryPath := "/tmp/kubebuilder/bin/kubebuilder"


camilamacedo86

Just please rebase with the master, for we have only your specific change and squash the commits. The change shows great. So we can get this one merged.

/ok-to-test

camilamacedo86

Hi @yyy1000,

Thank you for your contribution. 🥇 Just a few nits and then it seems great to fly:

Remove changes in generate_samples.go, which are out of the scope of this PR and unnecessary and should not be made. We should not replace the const values that centralize the Kubebuilder binary with strings within the path.
Squash the commits

yyy1000 · 2023-05-07T05:52:47Z

Hi @yyy1000,

Thank you for your contribution. 🥇 Just a few nits and then it seems great to fly:

Remove changes in generate_samples.go, which are out of the scope of this PR and unnecessary and should not be made. We should not replace the const values that centralize the Kubebuilder binary with strings within the path.

Squash the commits

Fixed it already. Thanks!

camilamacedo86

/approved cancel

Hi @yyy1000,

Sorry but after re-check this one it still not quite right. See:

If none then it is right. The hack/header-file should not be scaffolded and we change the makefile target
However, it is missing:

I am a user. I would like to have the current behaviour done when I select none for a new optional flag with a name that make s it clearer (copyright)

missing criteria

yyy1000 · 2023-05-07T14:52:50Z

/approved cancel

Hi @yyy1000,

Sorry but after re-check this one it still not quite right. See:

If none then it is right. The hack/header-file should not be scaffolded and we change the makefile target

However, it is missing:

I am a user. I would like to have the current behaviour done when I select none for a new optional flag with a name that make s it clearer (copyright)

Sorry for that. I used to think that only applied to 'license' flag.
Do you mean we can add a new optional flag called 'copyright' and make the new optional flag has the same behavior like 'license'?

camilamacedo86

Hi @yyy1000,

Try to create a new project and use --license=none. You will see that it will create the header within the copyright.

So, the idea is:

a) none should be NONE. That means no license at all
b) we can add a new optional value for the flag license to allow a user inform --license=copyright to have the current behaviour.

Following the places where the change needs to be made. See:

kubebuilder/pkg/plugins/golang/v4/scaffolds/internal/templates/hack/boilerplate.go

Lines 100 to 111 in 7aa151a

    
           const boilerplateTemplate = `/* 
        
           {{ if .Owner -}} 
        
           Copyright {{ .Year }} {{ .Owner }}. 
        
           {{- else -}} 
        
           Copyright {{ .Year }}. 
        
           {{- end }} 
        
           {{ index .Licenses .License }}*/` 
        
           var knownLicenses = map[string]string{ 
        
           	"apache2": apache2, 
        
           	"none":    "", 
        
           }

kubebuilder/pkg/plugins/golang/v4/init.go

Lines 87 to 89 in 7aa151a

    
           fs.StringVar(&p.license, "license", "apache2", 
        
           	"license to use to boilerplate, may be one of 'apache2', 'none'") 
        
           fs.StringVar(&p.owner, "owner", "", "owner to add to the copyright")

Please, let me know if that make sense and feel free to reach out if you have questions.

feat: change the binaryPath when update tutorial fix: revert const name fix: current behavior

yyy1000 · 2023-05-08T03:01:14Z

Hi @yyy1000,

Try to create a new project and use --license=none. You will see that it will create the header within the copyright.

So, the idea is:

a) none should be NONE. That means no license at all

b) we can add a new optional value for the flag license to allow a user inform --license=copyright to have the current behaviour.

Following the places where the change needs to be made. See:

kubebuilder/pkg/plugins/golang/v4/scaffolds/internal/templates/hack/boilerplate.go

Lines 100 to 111 in 7aa151a

const boilerplateTemplate = `/*

{{ if .Owner -}}

Copyright {{ .Year }} {{ .Owner }}.

{{- else -}}

Copyright {{ .Year }}.

{{- end }}

{{ index .Licenses .License }}*/`

var knownLicenses = map[string]string{

"apache2": apache2,

"none": "",

}

kubebuilder/pkg/plugins/golang/v4/init.go

Lines 87 to 89 in 7aa151a

fs.StringVar(&p.license, "license", "apache2",

"license to use to boilerplate, may be one of 'apache2', 'none'")

fs.StringVar(&p.owner, "owner", "", "owner to add to the copyright")

Please, let me know if that make sense and feel free to reach out if you have questions.

Sorry for missing that :) Thanks for your help, I have fixed it.

camilamacedo86 · 2023-05-08T05:06:39Z

pkg/plugins/golang/v4/scaffolds/internal/templates/hack/boilerplate.go

@@ -106,8 +106,8 @@ Copyright {{ .Year }}.
 {{ index .Licenses .License }}*/`

 var knownLicenses = map[string]string{
-	"apache2": apache2,
-	"none":    "",


@yyy1000,

If we remove none as an option, then it will fail when used. See:

kubebuilder/pkg/plugins/golang/v4/scaffolds/internal/templates/hack/boilerplate.go

Lines 51 to 63 in 7aa151a

func (f Boilerplate) Validate() error {

if f.License == "" {

// A default license will be set later

} else if _, found := knownLicenses[f.License]; found {

// One of the know licenses

} else if _, found := f.Licenses[f.License]; found {

// A map containing the requested license was also provided

} else {

return fmt.Errorf("unknown specified license %s", f.License)

}

return nil

}

If we keep none and copyright within the value "" instead of a const, we need to ensure the behaviour and that when we use one or another, the expected output will occur.

If we do not inform the tag license, then apache2 should be outputted by default.

Did you test it out locally? Following the steps:

If the license is not informed, we should keep the same current behaviour output apache2 as default.

Create a new project: $ kubebuilder init

Create a new API: $ kubebuilder create api --group ship --version v1beta1 --kind Frigate

Check the header of any go file. $ cat internal/controller/suite_test.go

It should have outputted apache2

If we use --license=apache2, we should the apache2 as above

Create a new project: $ kubebuilder init --license=apache2

Create a new API: $ kubebuilder create api --group ship --version v1beta1 --kind Frigate

Check the header of any go file. $ cat internal/controller/suite_test.go

If we use --license=none, we should not scaffold hack/bolerplate.txt, and we should not scaffold the path in the Makefile file target

Create a new project: $ kubebuilder init --license=none

Create a new API: $ kubebuilder create api --group ship --version v1beta1 --kind Frigate

Check the header of any go file. $ cat internal/controller/suite_test.go

If we use --license=copyright, then we should scaffold hack/bolerplate.txt and have only the header with the copyright . See:

kubebuilder/pkg/plugins/golang/v4/scaffolds/internal/templates/hack/boilerplate.go

Lines 100 to 106 in 7aa151a

const boilerplateTemplate = `/*

{{ if .Owner -}}

Copyright {{ .Year }} {{ .Owner }}.

{{- else -}}

Copyright {{ .Year }}.

{{- end }}

{{ index .Licenses .License }}*/`

Well, I think it's OK to delete the option 'none'. If we use the option 'none', it will not execute scaffold.Execute(bpFile). So knownLicenses will not be used.

So, how none can be valid and not exist in knownLicenses[f.License]?

yyy1000 · 2023-05-08T05:17:12Z

pkg/plugins/golang/v4/scaffolds/internal/templates/hack/boilerplate.go

-	"apache2": apache2,
-	"none":    "",
+	"apache2":   apache2,
+	"copyright": "",


I think that's right. We can define the const value 'copyright' a empty string. Just like const copyright = ""?

camilamacedo86 · 2023-05-08T05:27:43Z

pkg/plugins/golang/v4/scaffolds/internal/templates/hack/boilerplate.go

-	"apache2": apache2,
-	"none":    "",
+	"apache2":   apache2,
+	"copyright": "",


We cannot have any copyright withing the value of none.
I think we might need to check if is possible to pass here a const copyright wich will be == boilerplateTemplate

Actually I think the 'const copyright' == “”（an empty string) is enough. the {{ index .Licenses .License }}*/ ` will just be an empty string.

yyy1000 · 2023-05-08T09:00:19Z

As I tested it, it works OK now. Can we continue to merge it or should I add some tests in the PR first?

yyy1000 · 2023-05-08T10:26:45Z

pkg/plugins/golang/v4/scaffolds/init.go

+			Owner:   s.owner,
+		}
+		bpFile.Path = s.boilerplatePath
+		if err := scaffold.Execute(bpFile); err != nil {


well, that's a good question. 'none' is valid even if not exist in knownLicenses[f.License].
Let me explain it for you.
If we pass '--license=none', then it will skip this(line 90).
In scaffold.Execute(bpFile), it will call SetTemplateDefaults() and Validate(). So if we set 'none', it will never use knownLicenses[f.License]. Does it make sense? @camilamacedo86

camilamacedo86

It seems OK. Thank you for your contribution 🥇

/approve

k8s-ci-robot · 2023-05-08T10:29:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, 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]

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

camilamacedo86 · 2023-05-08T10:31:08Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 6, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 6, 2023

k8s-ci-robot requested review from camilamacedo86 and pwittrock May 6, 2023 11:28

This was referenced May 6, 2023

'make generate' can't work without kubebuilder binary #3390

Closed

📖: Update go version in docs to v1.20 #3389

Merged

camilamacedo86 reviewed May 6, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 6, 2023

camilamacedo86 requested changes May 7, 2023

View reviewed changes

yyy1000 force-pushed the fix-license branch from df6e837 to 4324550 Compare May 7, 2023 05:50

camilamacedo86 previously approved these changes May 7, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2023

camilamacedo86 added this to the v3.11.0 milestone May 7, 2023

camilamacedo86 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2023

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2023

camilamacedo86 reviewed May 7, 2023

View reviewed changes

feat: scaffold when lincense type none

8b81fd1

feat: change the binaryPath when update tutorial fix: revert const name fix: current behavior

yyy1000 force-pushed the fix-license branch from 4324550 to 8b81fd1 Compare May 8, 2023 02:59

camilamacedo86 reviewed May 8, 2023

View reviewed changes

yyy1000 commented May 8, 2023

View reviewed changes

camilamacedo86 reviewed May 8, 2023

View reviewed changes

yyy1000 commented May 8, 2023

View reviewed changes

camilamacedo86 approved these changes May 8, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2023

k8s-ci-robot assigned camilamacedo86 May 8, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2023

k8s-ci-robot merged commit 1fa2170 into kubernetes-sigs:master May 8, 2023

yyy1000 deleted the fix-license branch May 8, 2023 11:15

yyy1000 mentioned this pull request Aug 29, 2023

REQUEST: New membership for yyy1000 kubernetes/org#4417

Closed

9 tasks

camilamacedo86 mentioned this pull request Nov 1, 2023

Failed to create multi-version API #3672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛: (go/v4) Scaffold no header when lincense is type none #3391

🐛: (go/v4) Scaffold no header when lincense is type none #3391

yyy1000 commented May 6, 2023 •

edited

Loading

k8s-ci-robot commented May 6, 2023

camilamacedo86 May 6, 2023

yyy1000 May 6, 2023

camilamacedo86 May 6, 2023 •

edited

Loading

yyy1000 May 6, 2023 •

edited

Loading

camilamacedo86 May 7, 2023

camilamacedo86 May 6, 2023

camilamacedo86 left a comment

camilamacedo86 left a comment

yyy1000 commented May 7, 2023

camilamacedo86 left a comment •

edited

Loading

yyy1000 commented May 7, 2023 •

edited

Loading

camilamacedo86 left a comment

yyy1000 commented May 8, 2023

camilamacedo86 May 8, 2023 •

edited

Loading

yyy1000 May 8, 2023

camilamacedo86 May 8, 2023

yyy1000 May 8, 2023

camilamacedo86 May 8, 2023 •

edited

Loading

yyy1000 May 8, 2023

yyy1000 commented May 8, 2023

yyy1000 May 8, 2023

camilamacedo86 left a comment

k8s-ci-robot commented May 8, 2023

camilamacedo86 commented May 8, 2023

	const boilerplateTemplate = `/*
	{{ if .Owner -}}
	Copyright {{ .Year }} {{ .Owner }}.
	{{- else -}}
	Copyright {{ .Year }}.
	{{- end }}
	{{ index .Licenses .License }}*/`

	var knownLicenses = map[string]string{
	"apache2": apache2,
	"none": "",
	}

	fs.StringVar(&p.license, "license", "apache2",
	"license to use to boilerplate, may be one of 'apache2', 'none'")
	fs.StringVar(&p.owner, "owner", "", "owner to add to the copyright")

	func (f Boilerplate) Validate() error {
	if f.License == "" {
	// A default license will be set later
	} else if _, found := knownLicenses[f.License]; found {
	// One of the know licenses
	} else if _, found := f.Licenses[f.License]; found {
	// A map containing the requested license was also provided
	} else {
	return fmt.Errorf("unknown specified license %s", f.License)
	}

	return nil
	}

🐛: (go/v4) Scaffold no header when lincense is type none #3391

🐛: (go/v4) Scaffold no header when lincense is type none #3391

Conversation

yyy1000 commented May 6, 2023 • edited Loading

k8s-ci-robot commented May 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 May 6, 2023 • edited Loading

Choose a reason for hiding this comment

yyy1000 May 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

yyy1000 commented May 7, 2023

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

yyy1000 commented May 7, 2023 • edited Loading

camilamacedo86 left a comment

Choose a reason for hiding this comment

yyy1000 commented May 8, 2023

camilamacedo86 May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyy1000 commented May 8, 2023

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 8, 2023

camilamacedo86 commented May 8, 2023

yyy1000 commented May 6, 2023 •

edited

Loading

camilamacedo86 May 6, 2023 •

edited

Loading

yyy1000 May 6, 2023 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

yyy1000 commented May 7, 2023 •

edited

Loading

camilamacedo86 May 8, 2023 •

edited

Loading

camilamacedo86 May 8, 2023 •

edited

Loading