✨ Continue to fix scaffolding hub and spoke interfaces #3058

momom-i · 2022-11-03T08:59:29Z

This PR is the rest of #2084.
Details more on above PR.

Related issue: #2589

momom-i · 2022-11-03T09:05:42Z

I don't understand how to fix https://github.com/kubernetes-sigs/kubebuilder/pull/2084/files#r599386603 and https://github.com/kubernetes-sigs/kubebuilder/pull/2084/files#r599404131.
It's my very first time to contribute to kubebuilder repo, so I'd appreciate it if someone would explain more on that comments. 🙇

camilamacedo86 · 2022-11-03T11:11:48Z

/ok-to-test

camilamacedo86 · 2022-11-03T11:13:12Z

pkg/plugins/golang/v3/scaffolds/webhook.go

+		resource:   resource,
+		force:      force,
+		doScaffold: doScaffold,
+		spoke:      spoke,


Why do we need to pass it here?
Could not the value be in the config?

I think doScaffold and spoke come from option when user type like kubebuilder create webhook --group crew --version v1 --kind FirstKind --conversion --hub v1 --spoke v2 and these can't be gotten from config.
Is my understanding right?

camilamacedo86 · 2022-11-03T11:13:48Z

pkg/plugins/golang/v3/webhook.go

 	if !p.resource.HasDefaultingWebhook() && !p.resource.HasValidationWebhook() && !p.resource.HasConversionWebhook() {
 		return fmt.Errorf("%s create webhook requires at least one of --defaulting,"+
-			" --programmatic-validation and --conversion to be true", p.commandName)
+			" --programmatic-validation, --conversion, and --spoke to be provided", p.commandName)


Why and? Is not and/OR

I'm not good at English so I just reflected this comment. But as you said, I think spoke option is optional so likefmt.Errorf("%s create webhook requires at least one of --defaulting,"+" --programmatic-validation and --conversion to be true. If using conversion webhook"+ "you can specify the spoke versions in the command.", p.commandName) is better?

Is possible to use --conversion with spoke and --programmatic-validation with spoke or not?
So, I think we need to clarify it in the text.

camilamacedo86 · 2022-11-03T11:14:43Z

test/testdata/generate.sh

@@ -59,7 +59,14 @@ function scaffold_test_project {
    else
      $kb create api --group crew --version v1 --kind FirstMate --controller=true --resource=true --make=false
    fi
-    $kb create webhook --group crew --version v1 --kind FirstMate --conversion


We need to that for project-v4 as well.

camilamacedo86 · 2022-11-03T11:15:37Z

testdata/project-v3-config/PROJECT

@@ -16,6 +16,8 @@ resources:
  version: v1
  webhooks:
    defaulting: true
+    spokes:
+    - ""


Why the version? What does mean the version?
What are the valid version options?
Should not it be a bool with true like the other ones?

@momom-i

@camilamacedo86 @varshaprasad96
#3058 (comment)
As you said before, I think we can just take the boolean option whether it is hub or spoke and create Hub(), ConvertTo() or ConvertFrom() template. For instance kubebuilder create webhook --version v1 --hub.
What do you think?

Note that kubebuilder create webhook --version v1 --hub. The version is for teh webhook and not spoke, So, why are you adding an version to spoke? Look how the others options for webhooks are tracked. Example conversion: true . So, it shows for me that should be a boolean.

@camilamacedo86
I agree with you. And I've not changed yet since @/varshaprasad96' PR. I just fixed the points where the reviewers mentioned in that PR.
And I'd like to confirm to you whether I can change this PR to take only one boolean option or not in advance because it's going to be a big change.
As I mentioned before, example usage I'm supposed is like kubebuilder create webhook --version v1 --conversion --hub or kubebuilder create webhook --version v1 --conversion --spoke.

If user entered kubebuilder create webhook --version v1 --hub(it implies conversion is true) , it will create Hub() skeleton code under project/api/v1/{resource}_conversion.go.
If user entered kubebuilder create webhook --version v2 --spoke , it will create ConvertTo() or ConvertFrom() skeleton code under project/api/v2/{resource}_conversion.go.

camilamacedo86

/hold

Until we are able to address all changes and review.

k8s-ci-robot · 2022-11-03T11:22:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: momom-i
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval by writing /assign @varshaprasad96 in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

k8s-triage-robot · 2022-11-03T13:23:50Z

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

varshaprasad96 · 2022-11-16T20:16:58Z

pkg/config/v2/config.go

@@ -189,6 +189,20 @@ func (c cfg) GetResources() ([]resource.Resource, error) {
 	return resources, nil
 }

+// ListResourcesWithGK implements config.Config
+func (c cfg) ListResourcesWithGK(gvk resource.GVK) []resource.Resource {


wondering if we need these changes in v2 plugin, since we will be deprecating it soon.

We should not change the v2 +1 Good Point.

I understand.

varshaprasad96 · 2022-11-16T20:18:55Z

pkg/plugins/golang/v3/scaffolds/internal/templates/api/conversation.go

+	machinery.ResourceMixin
+
+	// Version refers to the conversion webhook versions for hub and spoke.
+	Version string


Could you please elaborate on how the command would like like? I remember there were many questions and discussions on the previous PR regarding the same. It will really helpful to revisit the command UX.

momom-i · 2022-12-26T13:14:41Z

@camilamacedo86 @varshaprasad96
#3058 (comment)
As you said before, I think we can just take the boolean option whether it is hub or spoke and create Hub(), ConvertTo() or ConvertFrom() template. For instance kubebuilder create webhook --version v1 --hub.
What do you think?

k8s-ci-robot · 2023-01-10T05:36:48Z

@momom-i: PR needs rebase.

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.

k8s-ci-robot · 2023-01-10T22:54:53Z

@momom-i: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubebuilder-e2e-k8s-1-24-7	`068d9a1`	link	true	`/test pull-kubebuilder-e2e-k8s-1-24-7`
pull-kubebuilder-e2e-k8s-1-23-13	`068d9a1`	link	true	`/test pull-kubebuilder-e2e-k8s-1-23-13`
pull-kubebuilder-e2e-k8s-1-25-3	`068d9a1`	link	true	`/test pull-kubebuilder-e2e-k8s-1-25-3`
pull-kubebuilder-e2e-k8s-1-26-0	`068d9a1`	link	true	`/test pull-kubebuilder-e2e-k8s-1-26-0`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

momom-i · 2023-01-11T13:26:31Z

@camilamacedo86 @varshaprasad96

#3058 (comment)

momom-i · 2023-02-04T08:18:33Z

I'll close this PR because I recreated the PR. #3201

Fix scaffolding hub and spoke interfaces

b0ae920

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 3, 2022

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 3, 2022

camilamacedo86 reviewed Nov 3, 2022

View reviewed changes

camilamacedo86 requested changes Nov 3, 2022

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2022

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 3, 2022

momom-i added 2 commits November 6, 2022 11:09

Fix nits

35b9479

Fix to add v4

068d9a1

momom-i force-pushed the conversion-webhook branch from 1d62f44 to 068d9a1 Compare November 8, 2022 16:00

varshaprasad96 reviewed Nov 16, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023

momom-i mentioned this pull request Feb 4, 2023

✨ Continue to fix scaffolding hub and spoke interfaces #3201

Closed

momom-i closed this Feb 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Continue to fix scaffolding hub and spoke interfaces #3058

✨ Continue to fix scaffolding hub and spoke interfaces #3058

momom-i commented Nov 3, 2022 •

edited

Loading

momom-i commented Nov 3, 2022

camilamacedo86 commented Nov 3, 2022

camilamacedo86 Nov 3, 2022

momom-i Nov 5, 2022

camilamacedo86 Nov 3, 2022

momom-i Nov 6, 2022

camilamacedo86 Jan 11, 2023

camilamacedo86 Nov 3, 2022

camilamacedo86 Nov 3, 2022

camilamacedo86 Jan 11, 2023

momom-i Jan 12, 2023

camilamacedo86 left a comment

k8s-ci-robot commented Nov 3, 2022

k8s-triage-robot commented Nov 3, 2022

varshaprasad96 Nov 16, 2022

camilamacedo86 Nov 16, 2022

momom-i Nov 29, 2022

varshaprasad96 Nov 16, 2022

momom-i commented Dec 26, 2022 •

edited

Loading

k8s-ci-robot commented Jan 10, 2023

k8s-ci-robot commented Jan 10, 2023

momom-i commented Jan 11, 2023

momom-i commented Feb 4, 2023

✨ Continue to fix scaffolding hub and spoke interfaces #3058

✨ Continue to fix scaffolding hub and spoke interfaces #3058

Conversation

momom-i commented Nov 3, 2022 • edited Loading

momom-i commented Nov 3, 2022

camilamacedo86 commented Nov 3, 2022

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

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

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 3, 2022

k8s-triage-robot commented Nov 3, 2022

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

momom-i commented Dec 26, 2022 • edited Loading

k8s-ci-robot commented Jan 10, 2023

k8s-ci-robot commented Jan 10, 2023

momom-i commented Jan 11, 2023

momom-i commented Feb 4, 2023

momom-i commented Nov 3, 2022 •

edited

Loading

momom-i commented Dec 26, 2022 •

edited

Loading