✨use bundle options struct for creating new bundles #3379

NikhilSharmaWe · 2023-05-01T12:51:05Z

Description

This PR uses bundle options struct instead of funcs for creating new bundles through NewBundleWithOptions function.

Motivation

k8s-ci-robot · 2023-05-01T12:51:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: NikhilSharmaWe
Once this PR has been reviewed and has the lgtm label, please assign pwittrock 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

varshaprasad96 · 2023-05-01T14:16:31Z

cmd/main.go

@@ -37,26 +37,21 @@ import (
 	grafanav1alpha1 "sigs.k8s.io/kubebuilder/v3/pkg/plugins/optional/grafana/v1alpha"
 )

+// nolint:lll


Any specific reason for adding this?

varshaprasad96 · 2023-05-01T14:18:02Z

pkg/plugin/bundle.go

+type BundlePlugin struct {
+	Options struct {
+		name                     string
+		version                  Version
+		plugins                  []Plugin
+		supportedProjectVersions []config.Version
+		deprecateWarning         string
 	}
 }


Wondering if we need the nested struct. Wdyt of making it this?

Suggested change

type BundlePlugin struct {

Options struct {

name string

version Version

plugins []Plugin

supportedProjectVersions []config.Version

deprecateWarning string

}

}

type BundlePluginOptions struct {

name string

version Version

plugins []Plugin

supportedProjectVersions []config.Version

deprecateWarning string

}

varshaprasad96 · 2023-05-01T14:19:30Z

pkg/plugin/bundle.go

+func NewBundlePlugin() BundlePlugin {
+	return BundlePlugin{}


So if BundlePlugin is exported, then there is no need of calling NewBundlePlugin func. We can make the BundlePlugin not to be exported (bundlePlugin) and have this func.

varshaprasad96 · 2023-05-01T14:20:05Z

pkg/plugin/bundle.go

-	allPlugins := make([]Plugin, 0, len(bundleOpts.plugins))
-	for _, plugin := range bundleOpts.plugins {
+	allPlugins := make([]Plugin, 0, len(bp.Options.plugins))
+	for _, plugin := range bp.Options.plugins {


Was there a specific reason to make a nested struct?

@varshaprasad96 Sorry about that. Changed it.

Actually, according to me the changes I made in this #3312 for #3307 were reasonable, since it implemented the functional arguments for the NewBundleWithOptions func for creating Bundles to prevent the long function signature for the NewBundle func, which achieves the goal for which the issue was made.

But @camilamacedo86 has suggested that using struct for bundle options is more reasonable.

But with the calling the function for bundle creation becomes very long like

kubebuilder/cmd/main.go

Line 50 in 0ceec77

gov3Bundle, _ := bpgov3.WithName(golang.DefaultNameQualifier).WithVersion(plugin.Version{Number: 3}).WithDeprecationMessage(deprecateMessageGoV3Bundle).WithPlugins(kustomizecommonv1.Plugin{}, golangv3.Plugin{}).NewBundleWithOptions()

Which also makes the lint test fails to have to skip them, therefore #3379 (comment).

Please inform me if my changes are reasonable and why this approach is better than #3312. Maybe I am missing here something.

camilamacedo86 · 2023-05-01T14:57:03Z

cmd/main.go

-		plugin.WithPlugins(kustomizecommonv1.Plugin{}, golangv3.Plugin{}),
-	)
+	bpgov3 := plugin.NewBundlePluginOptions()
+	gov3Bundle, _ := bpgov3.WithName(golang.DefaultNameQualifier).WithVersion(plugin.Version{Number: 3}).WithDeprecationMessage(deprecateMessageGoV3Bundle).WithPlugins(kustomizecommonv1.Plugin{}, golangv3.Plugin{}).NewBundleWithOptions()


Suggested change

gov3Bundle, _ := bpgov3.WithName(golang.DefaultNameQualifier).WithVersion(plugin.Version{Number: 3}).WithDeprecationMessage(deprecateMessageGoV3Bundle).WithPlugins(kustomizecommonv1.Plugin{}, golangv3.Plugin{}).NewBundleWithOptions()

gov3Bundle, _ := bpgov3.WithName(golang.DefaultNameQualifier).WithVersion(plugin.Version{Number: 3}).WithDeprecationMessage(deprecateMessageGoV3Bundle).WithPlugins(kustomizecommonv1.Plugin{}, golangv3.Plugin{}).Build()

camilamacedo86 · 2023-05-01T14:57:30Z

cmd/main.go

-		plugin.WithDeprecationMessage(deprecateMessageGoV3Bundle),
-		plugin.WithPlugins(kustomizecommonv1.Plugin{}, golangv3.Plugin{}),
-	)
+	bpgov3 := plugin.NewBundlePluginOptions()


Suggested change

bpgov3 := plugin.NewBundlePluginOptions()

gov3 := plugin.NewBundlePluginBuilder()

camilamacedo86 · 2023-05-01T14:57:49Z

pkg/plugin/bundle.go

@@ -31,31 +31,35 @@ type bundle struct {
 	deprecateWarning         string
 }

-type BundleOption func(*bundle)
+type BundlePluginOptions struct {


Suggested change

type BundlePluginOptions struct {

type BundlePluginBuilder struct {

camilamacedo86 · 2023-05-01T14:58:11Z

pkg/plugin/bundle.go

-	return func(opts *bundle) {
-		opts.name = name
-	}
+func NewBundlePluginOptions() BundlePluginOptions {


Suggested change

func NewBundlePluginOptions() BundlePluginOptions {

func NewBundlePluginBuilder() BundlePluginOptions {

camilamacedo86 · 2023-05-01T14:59:09Z

pkg/plugin/bundle.go

-	}
-
-	supportedProjectVersions := CommonSupportedProjectVersions(bundleOpts.plugins...)
+func (bp *BundlePluginOptions) NewBundleWithOptions() (Bundle, error) {


Suggested change

func (bp *BundlePluginOptions) NewBundleWithOptions() (Bundle, error) {

func (bp *BundlePluginOptions) Build() (Bundle, error) {

Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>

k8s-ci-robot · 2023-05-01T15:28:49Z

@NikhilSharmaWe: The following test 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-26-0	`0d0b341`	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.

NikhilSharmaWe · 2023-05-01T15:47:32Z

No need for this.
The current implementation is reasonable and probably better.

k8s-ci-robot requested a review from joelanford May 1, 2023 12:51

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

k8s-ci-robot requested a review from Kavinjsir May 1, 2023 12:51

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 1, 2023

NikhilSharmaWe marked this pull request as draft May 1, 2023 12:53

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 1, 2023

NikhilSharmaWe marked this pull request as ready for review May 1, 2023 12:56

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 1, 2023

k8s-ci-robot requested review from camilamacedo86 and everettraven May 1, 2023 12:56

varshaprasad96 reviewed May 1, 2023

View reviewed changes

NikhilSharmaWe force-pushed the upgradeBundle branch 3 times, most recently from 87da472 to 40e794a Compare May 1, 2023 14:44

camilamacedo86 reviewed May 1, 2023

View reviewed changes

NikhilSharmaWe force-pushed the upgradeBundle branch from 40e794a to 5dacc46 Compare May 1, 2023 15:01

use bundle options struct for creating new bundles

0d0b341

Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>

NikhilSharmaWe force-pushed the upgradeBundle branch from 5dacc46 to 0d0b341 Compare May 1, 2023 15:05

NikhilSharmaWe closed this May 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨use bundle options struct for creating new bundles #3379

✨use bundle options struct for creating new bundles #3379

NikhilSharmaWe commented May 1, 2023

k8s-ci-robot commented May 1, 2023

varshaprasad96 May 1, 2023

varshaprasad96 May 1, 2023

varshaprasad96 May 1, 2023

varshaprasad96 May 1, 2023

NikhilSharmaWe May 1, 2023 •

edited

Loading

camilamacedo86 May 1, 2023

camilamacedo86 May 1, 2023

camilamacedo86 May 1, 2023

camilamacedo86 May 1, 2023

camilamacedo86 May 1, 2023

k8s-ci-robot commented May 1, 2023

NikhilSharmaWe commented May 1, 2023

	gov3Bundle, _ := bpgov3.WithName(golang.DefaultNameQualifier).WithVersion(plugin.Version{Number: 3}).WithDeprecationMessage(deprecateMessageGoV3Bundle).WithPlugins(kustomizecommonv1.Plugin{}, golangv3.Plugin{}).NewBundleWithOptions()
	gov3Bundle, _ := bpgov3.WithName(golang.DefaultNameQualifier).WithVersion(plugin.Version{Number: 3}).WithDeprecationMessage(deprecateMessageGoV3Bundle).WithPlugins(kustomizecommonv1.Plugin{}, golangv3.Plugin{}).Build()

	bpgov3 := plugin.NewBundlePluginOptions()
	gov3 := plugin.NewBundlePluginBuilder()

	type BundlePluginOptions struct {
	type BundlePluginBuilder struct {

	func NewBundlePluginOptions() BundlePluginOptions {
	func NewBundlePluginBuilder() BundlePluginOptions {

	func (bp *BundlePluginOptions) NewBundleWithOptions() (Bundle, error) {
	func (bp *BundlePluginOptions) Build() (Bundle, error) {

✨use bundle options struct for creating new bundles #3379

✨use bundle options struct for creating new bundles #3379

Conversation

NikhilSharmaWe commented May 1, 2023

Description

Motivation

k8s-ci-robot commented May 1, 2023

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

NikhilSharmaWe May 1, 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

k8s-ci-robot commented May 1, 2023

NikhilSharmaWe commented May 1, 2023

NikhilSharmaWe May 1, 2023 •

edited

Loading