Default broker selection in CR #184

aliok · 2020-04-09T14:02:07Z

Issue to be fixed

Fixes #175

Proposed Changes

New DefaultBrokerClass in KnativeEventing CR
Make the given broker default broker

Notes:

Overwrites the config-br-defaults configmap based on the value given on CR

TODO:

Manifest update (waiting until the changes from Move broker defaults into core eventing#2947 updates the file gs://knative-nightly/eventing/latest/eventing.yaml)
Tooling to get new manifest files
Tests
Verification instructions

Release Note

knative-prow-robot

@aliok: 2 warnings.

Details

In response to this:

Issue to be fixed

Fixes #175

Proposed Changes

New DefaultBrokerClass in KnativeEventing CR

Make the given broker default broker

Notes:

Overwrites the config-br-defaults configmap based on the value given on CR

TODO:

Manifest update (waiting until the changes from knative/eventing#2947 updates the file gs://knative-nightly/eventing/latest/eventing.yaml)

Tooling to get new manifest files

Tests

Verification instructions

Release Note

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.

knative-prow-robot · 2020-04-09T14:02:18Z

pkg/reconciler/knativeeventing/controller.go

-	reconcilerName      = "KnativeEventing"
+	controllerAgentName       = "knativeeventing-controller"
+	reconcilerName            = "KnativeEventing"
+	ChannelBasedBrokerClass   = "ChannelBasedBroker"


Golint comments: exported const ChannelBasedBrokerClass should have comment (or a comment on this block) or be unexported. More info.

knative-prow-robot · 2020-04-09T14:02:18Z

pkg/reconciler/knativeeventing/controller.go

-	MasterURL  = flag.String("master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")
-	Kubeconfig = flag.String("kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.")
+	recursive             = flag.Bool("recursive", false, "If filename is a directory, process all manifests recursively")
+	MasterURL             = flag.String("master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")


Golint comments: exported var MasterURL should have comment or be unexported. More info.

knative-prow-robot · 2020-04-09T14:02:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [aliok]

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

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor · 2020-04-09T14:03:09Z

pkg/reconciler/knativeeventing/controller.go

 import (
 	"context"
 	"flag"
+	"k8s.io/client-go/rest"


Format Go code:

Suggested change

"k8s.io/client-go/rest"

mattmoor · 2020-04-09T14:03:09Z

pkg/reconciler/knativeeventing/controller.go

 	"flag"
+	"k8s.io/client-go/rest"
 	"os"
 	"path/filepath"


Format Go code:

Suggested change

"path/filepath"

"path/filepath"

"k8s.io/client-go/rest"

matzew · 2020-04-09T14:06:57Z

pkg/apis/eventing/v1alpha1/knativeeventing_types.go

 	// +optional
 	Registry Registry `json:"registry,omitempty"`
+
+	// The default broker type to use for the brokers Knative creates.


The default broker class ? I mean, that's what it is :-)

happy to receive naming suggestions!

matzew · 2020-04-09T14:09:53Z

pkg/reconciler/knativeeventing/controller.go

+	recursive             = flag.Bool("recursive", false, "If filename is a directory, process all manifests recursively")
+	MasterURL             = flag.String("master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")
+	Kubeconfig            = flag.String("kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.")
+	AvailableBrokerClases = []string{ChannelBasedBrokerClass, MTChannelBasedBrokerClass}


not sure if available is "right" here.

There might be others "available" ? I think I still could install my "custom broker", and set its class on the CR, right ?

Perhaps built-in? 🤷‍♂️

hmm, never thought of the custom brokers.
good point. gonna change some things to support it

matzew · 2020-04-09T14:12:56Z

cmd/manager/kodata/knative-eventing/eventing.yaml

+          # to "false".  To opt namespaces out of Broker injection, label
+          # them with:
+          #    knative-eventing-injection: disabled
+          name: BROKER_INJECTION_DEFAULT


why is this here, twice ?

matzew · 2020-04-09T14:13:07Z

cmd/manager/kodata/knative-eventing/eventing.yaml

+          # to "false".  To opt namespaces out of Broker injection, label
+          # them with:
+          #    knative-eventing-injection: disabled
+          name: BROKER_INJECTION_DEFAULT


and here too

knative-prow-robot · 2020-04-09T14:19:22Z

@aliok: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-eventing-operator-upgrade-tests	`df9dc49`	link	`/test pull-knative-eventing-operator-upgrade-tests`
pull-knative-eventing-operator-integration-tests	`df9dc49`	link	`/test pull-knative-eventing-operator-integration-tests`

Full PR test history. Your PR dashboard.

Details

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.

aliok · 2020-04-09T14:38:54Z

I am rewriting this PR to support customer brokers as well. that will need a different approach

Reconciliation for default broker selection

df9dc49

knative-prow-robot added the do-not-merge/work-in-progress label Apr 9, 2020

googlebot added the cla: yes label Apr 9, 2020

knative-prow-robot added the size/XXL label Apr 9, 2020

knative-prow-robot reviewed Apr 9, 2020

View reviewed changes

knative-prow-robot requested review from Cynocracy and n3wscott April 9, 2020 14:02

knative-prow-robot added the approved label Apr 9, 2020

mattmoor reviewed Apr 9, 2020

View reviewed changes

matzew reviewed Apr 9, 2020

View reviewed changes

aliok closed this Apr 9, 2020

aliok mentioned this pull request Apr 9, 2020

Reconciliation for default broker selection #186

Merged

Default broker selection in CR #184

Default broker selection in CR #184

Uh oh!

Conversation

aliok commented Apr 9, 2020

Issue to be fixed

Proposed Changes

Release Note

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Issue to be fixed

Proposed Changes

Release Note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Apr 9, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliok Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Apr 9, 2020

Uh oh!

aliok commented Apr 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aliok Apr 9, 2020 •

edited

Loading