✨ Continue to fix scaffolding hub and spoke interfaces #3201

momom-i · 2023-02-04T08:17:16Z

This PR is based on #2084.
I opened the PR but I wanted to be simpler so recreated it.

To scaffold hub and spoke interface, command-line is supposed to be like the following.

kubebuilder create webhook --group crew --version v1 --kind FirstMate --conversion --hub
kubebuilder create webhook --group crew --version v2 --kind FirstMate --conversion --spoke

I will open another PR for document change.

Related issue: #2589

k8s-ci-robot · 2023-02-04T08:17:26Z

Hi @momom-i. 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.

test/testdata/generate.sh

testdata/project-v4-config/PROJECT

camilamacedo86 · 2023-02-04T11:07:58Z

/ok-to-test

camilamacedo86 · 2023-02-04T20:03:54Z

pkg/plugins/golang/v3/webhook.go

+	fs.BoolVar(&p.options.DoHubScaffold, "hub", false,
+		"if set, scaffold ConvertTo function for Hub conversion")
+	fs.BoolVar(&p.options.DoSpokeScaffold, "spoke", false,
+		"if set, scaffold ConvertFrom function for Spoke conversion")


Could we have a hub and spoke interfaces for the other types that are not the Conversion ones?
If not, what about we add a validation?

Also, can we have both spoke and hub as true?

You mean if you set spoke or hub flags true, conversion flag should be true, right?
I don’t understand exactly what conversion flag do besides it prints messages that says you have to implement Hub and Convertible interface here. If my understanding right, there is another option to eliminate conversion flag, I think.

As you mentioned, we can set both values true and might have to change that.

camilamacedo86 · 2023-02-04T20:04:58Z

testdata/project-v4/api/v1/firstmate_conversion.go

+package v1
+
+// Hub marks that a given type is the hub type for conversion. -- only the no-op method 'Hub()' is required.
+// See https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/conversion#Hub.


Should we also add a link for https://book.kubebuilder.io/multiversion-tutorial/conversion.html?

I agree with that. Adding it makes users helpful more.

camilamacedo86 · 2023-02-04T20:11:11Z

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

@@ -0,0 +1,76 @@
+package api


All go files must to have the license:

See that the check if failing because of that

./test/check-license.sh
Checking for license header...
license header checking failed:

Could you please copy and paste from other ones to here?

Fixed it at 6947076 and 8d30477

camilamacedo86

That seems great. I just added some nits and questions
You did a terrific work 🥇

I think would be very nice to get a review from @varshaprasad96 on this one.

camilamacedo86 · 2023-02-05T07:24:22Z

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

+			return err
+		}
+	}
+
 	if doConversion {
 		fmt.Println(`Webhook server has been set up for you.


Could we not here add a condition to improve the text message?
It is valid only the options are not used?

fixed at 40988d1

camilamacedo86

HI @momom-i,

I will need some time to read the docs and check the initial PR to see if that is right or not.
I think we need to ensure that what is scaffold is according to the docs.

I will let you know more about soon as possible.

camilamacedo86 · 2023-02-21T08:42:43Z

Hi @momom-i,

It seems that we have an old design doc to achieve this goal see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/crd_version_conversion.md (a few things might be outdated because it was written prior we have the plugins ecosystem).

However, the initial idea was to scaffold the conversion appropriately. It is OK. We began from a version that still needs to do that. But we need to ensure that the base will allow us to grow to attend to these needs. So, it seems, for example, that we need to track in the PROJECT file and input what the GVK wants to convert instead of bool.

Could you please check the design doc and the documentation https://book.kubebuilder.io/multiversion-tutorial/conversion.html and ensure that this implementation addresses the basic required requirements?

momom-i · 2023-02-27T18:43:42Z

@camilamacedo86
Thank you for your response. I've read the documentation and confirmed it seemed this PR change met the required requirements.

So, it seems, for example, that we need to track in the PROJECT file and input what the GVK wants to convert instead of bool.

How can I get this requirement statement? Is there any documentation anywhere?

k8s-ci-robot · 2023-03-16T05:35:17Z

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.

momom-i · 2023-03-17T00:27:54Z

@camilamacedo86 👀

lauchokyip

@momom-i I read your PR and the OP's PR and seem like there are some discrepancies.

i) this important function is missing , instead of getting the resource from input, it actually gets it from the config file
ii) As this comment mentioned, we would want to make --version as the --spoke flag and completely remove --spoke flag

lauchokyip · 2023-05-10T04:20:39Z

pkg/plugins/golang/v4/scaffolds/internal/templates/api/conversion.go

+	}
+
+	c.Path = c.Resource.Replacer().Replace(c.Path)
+	fmt.Println(c.Path)


Do we need the fmt.Println?

k8s-ci-robot · 2023-05-10T04:28:06Z

[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. 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

lauchokyip · 2023-05-10T04:28:31Z

/hold

lauchokyip · 2023-05-11T12:59:11Z

@momom-i are you still interested in continue the effort? If not, I would love to dig deep into it 😄

momom-i · 2023-05-15T01:11:30Z

@lauchokyip I'm sorry for my delay response. I'd like to continue to work on this. 🙏
Let me take a time to confirm your comment

lauchokyip · 2023-05-15T01:26:28Z

@lauchokyip I'm sorry for my delay response. I'd like to continue to work on this. pray Let me take a time to confirm your comment

Sure, would you be able to provide a timeline?

lauchokyip · 2023-05-15T01:30:14Z

Also please reach out on kubebuilder Slack channel if you need any help or more visibility

k8s-triage-robot · 2024-01-21T12:20:57Z

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

This bot triages PRs 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 PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

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

/lifecycle stale

k8s-ci-robot · 2024-02-01T12:49:41Z

@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-27-1	`2dfce41`	link	true	`/test pull-kubebuilder-e2e-k8s-1-27-1`
pull-kubebuilder-e2e-k8s-1-28-6	`2dfce41`	link	true	`/test pull-kubebuilder-e2e-k8s-1-28-6`
pull-kubebuilder-e2e-k8s-1-27-10	`2dfce41`	link	true	`/test pull-kubebuilder-e2e-k8s-1-27-10`
pull-kubebuilder-e2e-k8s-1-28-0	`2dfce41`	link	true	`/test pull-kubebuilder-e2e-k8s-1-28-0`
pull-kubebuilder-e2e-k8s-1-27-3	`2dfce41`	link	true	`/test pull-kubebuilder-e2e-k8s-1-27-3`
pull-kubebuilder-e2e-k8s-1-26-6	`2dfce41`	link	true	`/test pull-kubebuilder-e2e-k8s-1-26-6`
pull-kubebuilder-e2e-k8s-1-29-0	`2dfce41`	link	true	`/test pull-kubebuilder-e2e-k8s-1-29-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.

k8s-triage-robot · 2024-03-02T13:02:20Z

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

This bot triages PRs 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 PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

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

/lifecycle rotten

k8s-triage-robot · 2024-04-01T13:21:05Z

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

This bot triages PRs 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 PR is closed

You can:

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

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

/close

k8s-ci-robot · 2024-04-01T13:21:09Z

@k8s-triage-robot: Closed this PR.

In response to this:

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

This bot triages PRs 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 PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

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

/close

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.

Fix to be simpler impl

64e2e3a

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

k8s-ci-robot requested review from joelanford and pwittrock February 4, 2023 08:17

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 4, 2023

momom-i mentioned this pull request Feb 4, 2023

✨ Continue to fix scaffolding hub and spoke interfaces #3058

Closed

Add test file

0fb9a97

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 4, 2023

camilamacedo86 reviewed Feb 4, 2023

View reviewed changes

test/testdata/generate.sh Show resolved Hide resolved

camilamacedo86 reviewed Feb 4, 2023

View reviewed changes

testdata/project-v4-config/PROJECT Show resolved Hide resolved

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 Feb 4, 2023

Fix

ee1478c

momom-i force-pushed the feat/add-conversion-scaffold branch from 91e74d7 to ee1478c Compare February 4, 2023 17:23

camilamacedo86 reviewed Feb 4, 2023

View reviewed changes

momom-i added 5 commits February 5, 2023 09:52

Fix not to pass hub and spoke args

e125128

Fix flag description

8700f2f

Add link

e6202aa

Add license

6947076

Regenerate testdata

8d30477

camilamacedo86 reviewed Feb 5, 2023

View reviewed changes

Add condition to print out conversion help message

40988d1

momom-i added 2 commits February 6, 2023 00:03

Add test

cf690cb

Add test and description more

2dfce41

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

lauchokyip suggested changes May 10, 2023

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 May 10, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2024

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 Mar 2, 2024

k8s-ci-robot closed this Apr 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Continue to fix scaffolding hub and spoke interfaces #3201

✨ Continue to fix scaffolding hub and spoke interfaces #3201

momom-i commented Feb 4, 2023

k8s-ci-robot commented Feb 4, 2023

camilamacedo86 commented Feb 4, 2023

camilamacedo86 Feb 4, 2023

camilamacedo86 Feb 4, 2023

momom-i Feb 5, 2023 •

edited

Loading

camilamacedo86 Feb 4, 2023

camilamacedo86 Feb 4, 2023

momom-i Feb 5, 2023

camilamacedo86 Feb 4, 2023

momom-i Feb 5, 2023

camilamacedo86 left a comment

camilamacedo86 Feb 5, 2023

momom-i Feb 5, 2023

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 commented Feb 21, 2023

momom-i commented Feb 27, 2023 •

edited

Loading

k8s-ci-robot commented Mar 16, 2023

momom-i commented Mar 17, 2023 •

edited

Loading

lauchokyip left a comment •

edited

Loading

lauchokyip May 10, 2023 •

edited

Loading

k8s-ci-robot commented May 10, 2023

lauchokyip commented May 10, 2023

lauchokyip commented May 11, 2023

momom-i commented May 15, 2023

lauchokyip commented May 15, 2023

lauchokyip commented May 15, 2023

k8s-triage-robot commented Jan 21, 2024

k8s-ci-robot commented Feb 1, 2024

k8s-triage-robot commented Mar 2, 2024

k8s-triage-robot commented Apr 1, 2024

k8s-ci-robot commented Apr 1, 2024

✨ Continue to fix scaffolding hub and spoke interfaces #3201

✨ Continue to fix scaffolding hub and spoke interfaces #3201

Conversation

momom-i commented Feb 4, 2023

k8s-ci-robot commented Feb 4, 2023

camilamacedo86 commented Feb 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

momom-i Feb 5, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 commented Feb 21, 2023

momom-i commented Feb 27, 2023 • edited Loading

k8s-ci-robot commented Mar 16, 2023

momom-i commented Mar 17, 2023 • edited Loading

lauchokyip left a comment • edited Loading

Choose a reason for hiding this comment

lauchokyip May 10, 2023 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented May 10, 2023

lauchokyip commented May 10, 2023

lauchokyip commented May 11, 2023

momom-i commented May 15, 2023

lauchokyip commented May 15, 2023

lauchokyip commented May 15, 2023

k8s-triage-robot commented Jan 21, 2024

k8s-ci-robot commented Feb 1, 2024

k8s-triage-robot commented Mar 2, 2024

k8s-triage-robot commented Apr 1, 2024

k8s-ci-robot commented Apr 1, 2024

momom-i Feb 5, 2023 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

momom-i commented Feb 27, 2023 •

edited

Loading

momom-i commented Mar 17, 2023 •

edited

Loading

lauchokyip left a comment •

edited

Loading

lauchokyip May 10, 2023 •

edited

Loading