feat(HACBS-2381): update service to use operator-toolkit #212

davidmogar · 2023-07-12T12:31:58Z

The operator-toolkit has been already released and operator-goodies is deprecated.

davidmogar · 2023-07-12T13:28:17Z

/retest

davidmogar · 2023-07-12T13:33:54Z

/retest

kasemAlem · 2023-07-13T08:12:14Z

/test release-service-e2e

The operator-toolkit has been already released and operator-goodies is deprecated. Signed-off-by: David Moreno García <[email protected]>

openshift-ci · 2023-07-13T13:28:14Z

New changes are detected. LGTM label has been removed.

johnbieren

Please remove the changes from this PR that are not related to switching to operator-toolkit. The PR says it is about using operator-toolkit. There should not be rewrites snuck into this

johnbieren · 2023-07-13T14:09:43Z

api/v1alpha1/webhooks/author/suite_test.go

I don't see how a single change in this file is related to "update service to use operator-toolkit"

Using operator-toolkit comes with declaring webhooks in a different way. That is, moving webhooks into new directories. Those new directories need a suite for tests to run.

johnbieren · 2023-07-13T14:10:55Z

api/v1alpha1/webhooks/author/webhook.go

I don't see how a single change in this file is related to "update service to use operator-toolkit"

Using operator-toolkit comes with declaring webhooks in a different way. That is, moving webhooks into new directories. Those new directories need the webhook code using the toolkit functions.

johnbieren · 2023-07-13T15:10:51Z

conditions/conditions_suite_test.go

This should be added to operator-toolkit before being removed

johnbieren · 2023-07-13T15:10:57Z

conditions/conditions_test.go

This should be added to operator-toolkit before being removed

johnbieren · 2023-07-13T15:12:57Z

api/v1alpha1/webhooks/author/webhook.go

@@ -29,35 +30,34 @@ import (
 	admissionv1 "k8s.io/api/admission/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	ctrlWebhook "sigs.k8s.io/controller-runtime/pkg/webhook"


How is this change necessary for adding toolkit and not a rewrite?

Because there's another webhook defined in the same package.

johnbieren · 2023-07-13T15:14:04Z

api/v1alpha1/webhooks/author/webhook.go

-// +kubebuilder:webhook:path=/mutate-appstudio-redhat-com-v1alpha1-author,mutating=true,failurePolicy=fail,sideEffects=None,groups=appstudio.redhat.com,resources=releases;releaseplans,verbs=create;update,versions=v1alpha1,name=mauthor.kb.io,admissionReviewVersions=v1
+// Register registers the webhook with the passed manager and log.
+func (w *Webhook) Register(mgr ctrl.Manager, log *logr.Logger) error {
+	w.client = mgr.GetClient()


How is this change necessary for adding toolkit and not a rewrite?
Why can't we just do

mgr.GetWebhookServer().Register("/mutate-appstudio-redhat-com-v1alpha1-author", &webhook.Admission{ Handler: &authorWebhook{ client: mgr.GetClient(), log: log.WithName("author"), }, })

and reduce the changeset?

Consistency. The information to pass has to be carried into the webhook struct and given that it's received as a receiver, the new code is better.

I do not think it is better, it is functionally the same and just increasing the diff for no benefit

johnbieren · 2023-07-13T15:14:16Z

api/v1alpha1/webhooks/author/webhook.go

 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )

-// authorWebhook describes the data structure for the author webhook
-type authorWebhook struct {


How is this change necessary for adding toolkit and not a rewrite?

Consistency across webhooks.

Then make a new PR called "consistency across webhooks" and add the hundreds of lines of changes related to it. Don't sneak it in this PR

johnbieren · 2023-07-13T15:15:51Z

api/v1alpha1/webhooks/author/webhook.go

 	labels := make(map[string]string)
 	if obj.GetLabels() != nil {
 		labels = obj.GetLabels()
 	}

-	labels[metadata.AuthorLabel] = a.sanitizeLabelValue(username)
+	labels[metadata.AuthorLabel] = w.sanitizeLabelValue(username)


How is this change necessary for adding toolkit and not a rewrite?

Consistency.

johnbieren · 2023-07-13T15:15:57Z

api/v1alpha1/webhooks/author/webhook.go

 	obj.SetLabels(labels)
 }

 // sanitizeLabelValue takes a username and returns it in a form appropriate to use as a label value.
-func (a *authorWebhook) sanitizeLabelValue(username string) string {
+func (w *Webhook) sanitizeLabelValue(username string) string {


How is this change necessary for adding toolkit and not a rewrite?

Consistency.

johnbieren · 2023-07-13T15:18:22Z

syncer/syncer.go

@@ -28,16 +28,16 @@ import (
 type Syncer struct {
 	client client.Client
 	ctx    context.Context
-	logger logr.Logger
+	logger *logr.Logger


How is this change necessary for adding toolkit and not a rewrite?

Because this comes from the adapter and we are following how the new adapter should be defined.

johnbieren · 2023-07-13T15:18:26Z

syncer/syncer.go

 }

 // NewSyncer creates a new Syncer with the given client and logger.
-func NewSyncer(client client.Client, logger logr.Logger) *Syncer {
+func NewSyncer(client client.Client, logger *logr.Logger) *Syncer {


How is this change necessary for adding toolkit and not a rewrite?

Because this comes from the adapter and we are following how the new adapter should be defined.

johnbieren · 2023-07-13T15:18:30Z

syncer/syncer.go

 	return NewSyncerWithContext(client, logger, context.TODO())
 }

 // NewSyncerWithContext creates a new Syncer with the given client, logger and context.
-func NewSyncerWithContext(client client.Client, logger logr.Logger, ctx context.Context) *Syncer {
+func NewSyncerWithContext(client client.Client, logger *logr.Logger, ctx context.Context) *Syncer {


How is this change necessary for adding toolkit and not a rewrite?

Because this comes from the adapter and we are following how the new adapter should be defined.

davidmogar requested review from theflockers and johnbieren July 12, 2023 12:31

davidmogar force-pushed the hacbs2381 branch 2 times, most recently from a71de3a to 7b0d0e8 Compare July 12, 2023 12:38

kasemAlem approved these changes Jul 13, 2023

View reviewed changes

openshift-ci bot assigned kasemAlem Jul 13, 2023

openshift-ci bot added the lgtm label Jul 13, 2023

feat(HACBS-2381): update service to use operator-toolkit

9be1a92

The operator-toolkit has been already released and operator-goodies is deprecated. Signed-off-by: David Moreno García <[email protected]>

davidmogar force-pushed the hacbs2381 branch from 7b0d0e8 to 9be1a92 Compare July 13, 2023 13:28

openshift-ci bot removed the lgtm label Jul 13, 2023

johnbieren requested changes Jul 13, 2023

View reviewed changes

openshift-ci bot assigned johnbieren Jul 13, 2023

johnbieren reviewed Jul 13, 2023

View reviewed changes

davidmogar closed this Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(HACBS-2381): update service to use operator-toolkit #212

feat(HACBS-2381): update service to use operator-toolkit #212

davidmogar commented Jul 12, 2023

davidmogar commented Jul 12, 2023

davidmogar commented Jul 12, 2023

kasemAlem commented Jul 13, 2023

openshift-ci bot commented Jul 13, 2023

johnbieren left a comment

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

johnbieren Jul 13, 2023

johnbieren Jul 13, 2023

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

johnbieren Jul 13, 2023

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

johnbieren Jul 13, 2023

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

johnbieren Jul 13, 2023

davidmogar Jul 13, 2023

feat(HACBS-2381): update service to use operator-toolkit #212

feat(HACBS-2381): update service to use operator-toolkit #212

Conversation

davidmogar commented Jul 12, 2023

davidmogar commented Jul 12, 2023

davidmogar commented Jul 12, 2023

kasemAlem commented Jul 13, 2023

openshift-ci bot commented Jul 13, 2023

johnbieren left a 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

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

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