User can specify the namespace in yaml for knative-eventing #711

zxDiscovery · 2019-01-10T06:41:50Z

Proposed Changes

User can set knative-eventing deployment namespace in yaml file.
The change is similar to knative/serving#2708

matzew · 2019-01-10T09:20:30Z

pkg/system/names.go

+
+// Namespace holds the K8s namespace where our eventing system
+// components run.
+func Namespace() string {


would it make sense to move this to some common location (e.g. to https://github.com/knative/pkg)

looks like relevant for serving, build etc as wll

Serving, building and eventing are all belong to knative. I think we can put the three into one namespace. So that we can use helm chart management easily.

+1 to @matzew , so that serving, building and eventing will not need to define this function but just import is enough.

I agree, it seems like this should be in knative/pkg.

Harwayne · 2019-01-10T17:11:39Z

/ok-to-test

zxDiscovery · 2019-01-11T06:21:22Z

/test pull-knative-eventing-go-coverage

zxDiscovery · 2019-01-11T06:25:32Z

/test pull-knative-eventing-go-coverage

Harwayne

This looks good to me, but I'll wait until it has merged into serving before LGTMing here, so that we can ensure the two implementations are in sync.

zxDiscovery · 2019-01-17T09:32:40Z

/test pull-knative-eventing-go-coverage

zxDiscovery · 2019-01-18T16:04:30Z

@Harwayne The PR knative/serving#2708 has already merged. Can we merge this PR now?

Harwayne

/lgtm
/hold
Holding for reasoning on why knative-testing is used in unit tests.

Harwayne · 2019-01-18T18:30:38Z

pkg/system/testing/names.go

+)
+
+func init() {
+	os.Setenv(system.NamespaceEnvKey, "knative-testing")


Why not knative-eventing?

Yeah, we want to put this in knative/pkg. Then the three component of knative will be use the same namespace. So here we change the namespace knative-testing to match. The serving and build component namespace is also knative-testing

Is the intention to put all knative pieces into a single namespace? Just in tests? Or in production as well?

I hope it in production and I'm doing this now. I think some other people also doing this(knative/serving#2623).

Harwayne · 2019-01-18T18:30:56Z

pkg/system/names.go

+
+// Namespace holds the K8s namespace where our eventing system
+// components run.
+func Namespace() string {


I agree, it seems like this should be in knative/pkg.

Harwayne · 2019-01-19T00:32:59Z

/hold cancel

zxDiscovery · 2019-02-12T09:39:32Z

test/test_images/logevents/main.go

@@ -29,7 +28,7 @@ type Heartbeat struct {

 func handler(ctx context.Context, data map[string]interface{}) {
 	metadata := cloudevents.FromContext(ctx)
-	log.Printf("[%s] %s %s: %+v", metadata.EventTime.Format(time.RFC3339), metadata.ContentType, metadata.Source, data)
+	log.Printf("[%s]: %+v", metadata.DataContentType(), data)


I found the latest event.go in knative/pkg change the function FromContext return value type from *EventContext to LoadContext. So there is no EventTime, ContentType and Source in metadata. For run this test, I change the log here. I'm not sure if here is right? @evankanderson @mattmoor

You should be able to do the following:

metadata := cloudevents.FromContext(ctx).AsV02() log.Printf("[%s] %s %s: %+v", metadata.Time.Format(time.RFC3339), metadata.ContentType, metadata.Source, data)

zxDiscovery · 2019-02-12T09:39:58Z

/test pull-knative-eventing-go-coverage

knative-metrics-robot · 2019-02-12T09:42:05Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
contrib/kafka/pkg/dispatcher/dispatcher.go	71.1%	70.1%	-1.0

evankanderson

/lgtm
/approve

knative-prow-robot · 2019-02-14T00:28:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, zxDiscovery

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson]

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

n3wscott · 2019-02-20T19:06:07Z

contrib/gcppubsub/config/gcppubsub.yaml

+              value: gcppubsub-channel-key
+            - name: DEFAULT_SECRET_KEY
+              value: key.json
+        env:


n3wscott · 2019-02-20T19:07:03Z

contrib/natss/config/provisioner.yaml

@@ -175,3 +180,8 @@ spec:
      containers:
        - name: dispatcher
          image: github.com/knative/eventing/contrib/natss/pkg/dispatcher
+        env:


indenting seems off

n3wscott · 2019-02-20T19:07:12Z

contrib/natss/config/provisioner.yaml

@@ -103,6 +103,11 @@ spec:
      containers:
        - name: controller
          image: github.com/knative/eventing/contrib/natss/pkg/controller
+        env:


indenting seems off

knative-prow-robot requested review from grantr and inlined January 10, 2019 06:41

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 10, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 10, 2019

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 10, 2019

zxDiscovery force-pushed the eventing-namespace branch 2 times, most recently from 2d1b7e3 to 09bbf3d Compare January 10, 2019 07:20

matzew reviewed Jan 10, 2019

View reviewed changes

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 10, 2019

zxDiscovery force-pushed the eventing-namespace branch 6 times, most recently from 90dae52 to 760c38e Compare January 11, 2019 05:54

Harwayne reviewed Jan 11, 2019

View reviewed changes

zxDiscovery force-pushed the eventing-namespace branch 2 times, most recently from 6c68d5b to fc3eeb5 Compare January 15, 2019 05:16

Harwayne reviewed Jan 18, 2019

View reviewed changes

knative-prow-robot assigned Harwayne Jan 18, 2019

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jan 18, 2019

zxDiscovery force-pushed the eventing-namespace branch from 4215cd8 to a4f4cf5 Compare January 19, 2019 00:30

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2019

zxDiscovery force-pushed the eventing-namespace branch 2 times, most recently from 911a070 to 8d0e8d1 Compare February 12, 2019 07:28

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 12, 2019

zxDiscovery force-pushed the eventing-namespace branch from 07b83c2 to bca8c7b Compare February 12, 2019 08:24

zxDiscovery commented Feb 12, 2019

View reviewed changes

zxDiscovery force-pushed the eventing-namespace branch from bca8c7b to f3cdceb Compare February 13, 2019 01:20

xu zhao added 6 commits February 14, 2019 07:57

User can specify the namespace in yaml for knative-eventing

7895947

change ns to knative-testing

e465b49

update the test ns in reconcile_test.go

c0a921a

change the test case for the new namespaces

f41362c

add clock.go in system

34fe9f2

Use knatve/pkg

3b0c0ba

zxDiscovery force-pushed the eventing-namespace branch from f3cdceb to 3b0c0ba Compare February 14, 2019 00:03

evankanderson reviewed Feb 14, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019

evankanderson mentioned this pull request Feb 14, 2019

Upgrade knative/pkg to latest #809

Closed

knative-prow-robot merged commit 0321cd0 into knative:master Feb 14, 2019

zxDiscovery deleted the eventing-namespace branch February 14, 2019 01:17

n3wscott reviewed Feb 20, 2019

View reviewed changes

contrib/gcppubsub/config/gcppubsub.yaml

value: gcppubsub-channel-key

- name: DEFAULT_SECRET_KEY

value: key.json

env:

Copy link

Contributor

n3wscott Feb 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops

n3wscott reviewed Feb 20, 2019

View reviewed changes

evankanderson pushed a commit that referenced this pull request Feb 20, 2019

Fix env vars from #711

5b312f7

evankanderson mentioned this pull request Feb 20, 2019

Fix env var indentation and grouping in contrib yamls #824

Merged

knative-prow-robot pushed a commit that referenced this pull request Feb 20, 2019

Fix env vars from #711 (#824)

3086593

evankanderson added a commit that referenced this pull request Feb 20, 2019

Fix env vars from #711 (#824)

540fa5f

evankanderson added a commit that referenced this pull request Feb 20, 2019

Fix env vars from #711 (#824)

86064bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User can specify the namespace in yaml for knative-eventing #711

User can specify the namespace in yaml for knative-eventing #711

zxDiscovery commented Jan 10, 2019 •

edited

Loading

matzew Jan 10, 2019

zxDiscovery Jan 10, 2019

gyliu513 Jan 16, 2019

Harwayne Jan 18, 2019

Harwayne commented Jan 10, 2019

zxDiscovery commented Jan 11, 2019

zxDiscovery commented Jan 11, 2019

Harwayne left a comment

zxDiscovery commented Jan 17, 2019

zxDiscovery commented Jan 18, 2019

Harwayne left a comment

Harwayne Jan 18, 2019

zxDiscovery Jan 19, 2019

Harwayne Jan 19, 2019

zxDiscovery Jan 19, 2019

Harwayne Jan 18, 2019

Harwayne commented Jan 19, 2019

zxDiscovery Feb 12, 2019

evankanderson Feb 13, 2019

zxDiscovery Feb 13, 2019

zxDiscovery commented Feb 12, 2019

knative-metrics-robot commented Feb 12, 2019

evankanderson left a comment

knative-prow-robot commented Feb 14, 2019

n3wscott Feb 20, 2019

n3wscott Feb 20, 2019

n3wscott Feb 20, 2019

User can specify the namespace in yaml for knative-eventing #711

User can specify the namespace in yaml for knative-eventing #711

Conversation

zxDiscovery commented Jan 10, 2019 • edited Loading

Proposed Changes

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

Harwayne commented Jan 10, 2019

zxDiscovery commented Jan 11, 2019

zxDiscovery commented Jan 11, 2019

Harwayne left a comment

Choose a reason for hiding this comment

zxDiscovery commented Jan 17, 2019

zxDiscovery commented Jan 18, 2019

Harwayne 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

Harwayne commented Jan 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zxDiscovery commented Feb 12, 2019

knative-metrics-robot commented Feb 12, 2019

evankanderson left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zxDiscovery commented Jan 10, 2019 •

edited

Loading