Skip to content

Channel and Subscription Readiness#1330

Merged
knative-prow-robot merged 22 commits into
knative:masterfrom
akashrv:1189
Jun 5, 2019
Merged

Channel and Subscription Readiness#1330
knative-prow-robot merged 22 commits into
knative:masterfrom
akashrv:1189

Conversation

@akashrv
Copy link
Copy Markdown
Contributor

@akashrv akashrv commented Jun 4, 2019 •
edited
Loading

Reference #1189

Proposed Changes

  • Extend Subscribable with SubscribableStatus
  • Update Subscription controller to update subscription status based on the SubscribableStatus if the channel
  • As a reference - Kafka Channel.
  • Existing channels will be affected by this change.

Sending early PR to get feedback as the changes are extensive.

To do:

  • E2E tests are not yet enabled.
  • Other channels (which can happen as they are being moved to operator model)

Release Note

akashrv added 13 commits May 29, 2019 17:41
@akashrv
Commit to pull from master
42e8a3a
@akashrv
Merge branch 'master' of github.com:knative/eventing into 1189
e80cc8d
@akashrv
WIP, before pulling channelable changes
b697348
@akashrv
(WIP) Merge branch 'master' of github.com:knative/eventing into 1189
dd1f22c
@akashrv
WIP - merged subscribable and addressable types
30f6f60
@akashrv
WIP before pulling changes from main
7070dce
@akashrv
Merge branch 'master' of github.com:knative/eventing into 1189
3044483
@akashrv
WIP before merging upstrea mmaster
625cf2c
@akashrv
Merge branch 'master' of github.com:knative/eventing into 1189
f6db117
@akashrv
WIP - before merging kafka SetAddress changes from upstream master
4396d13
@akashrv
Merge branch 'master' of github.com:knative/eventing into 1189
2b76447
@akashrv
WIP - Manual E2E test works for kafka channel.
ffd53aa 
todo: polish code, review, update uts, test more cases.
@akashrv
ready to send WIP PR
f776064
@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 4, 2019
@knative-prow-robot knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 4, 2019
@akashrv
Copy link
Copy Markdown
Contributor Author

akashrv commented Jun 4, 2019

/assign @Harwayne @vaikas-google @nachocano

vaikas
vaikas reviewed Jun 4, 2019
View reviewed changes
Comment thread contrib/kafka/pkg/apis/messaging/v1alpha1/kafka_channel_types.go Outdated
Comment thread pkg/apis/duck/v1alpha1/subscribable_types.go
Comment thread pkg/apis/duck/v1alpha1/subscribable_types.go
Comment thread pkg/apis/duck/v1alpha1/subscribable_types.go
Comment thread pkg/apis/duck/v1alpha1/subscribable_types.go Outdated
@akashrv akashrv changed the title [WIP] Channel and Subscription Readiness Channel and Subscription Readiness Jun 5, 2019
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2019
Harwayne
Harwayne reviewed Jun 5, 2019
View reviewed changes
Comment thread config/200-controller-clusterrole.yaml Outdated
Comment thread contrib/kafka/cmd/channel_controller/main.go
Comment thread contrib/kafka/pkg/dispatcher/dispatcher.go
Comment thread contrib/kafka/pkg/dispatcher/dispatcher.go
Comment thread contrib/kafka/pkg/reconciler/dispatcher/kafkachannel.go
Comment thread pkg/apis/duck/v1alpha1/subscribable_types.go
Comment thread pkg/apis/duck/v1alpha1/subscribable_types.go
Harwayne
Harwayne reviewed Jun 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@Harwayne Harwayne left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jun 5, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akashrv, Harwayne

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2019
@Harwayne
Copy link
Copy Markdown
Contributor

Harwayne commented Jun 5, 2019

/hold

Holding so that you can update the description before it is submitted.

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2019
@akashrv
Copy link
Copy Markdown
Contributor Author

akashrv commented Jun 5, 2019

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2019
@akashrv
Copy link
Copy Markdown
Contributor Author

akashrv commented Jun 5, 2019

/retest

@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Jun 5, 2019

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.0% 66.2% -4.8
pkg/apis/eventing/v1alpha1/subscription_lifecycle.go 71.4% 54.5% -16.9
pkg/apis/eventing/v1alpha1/test_helper.go 83.8% 84.2% 0.4
pkg/reconciler/subscription/subscription.go 79.2% 72.6% -6.6

@knative-prow-robot knative-prow-robot merged commit 7fae9c0 into knative:master Jun 5, 2019
@vaikas vaikas mentioned this pull request Jun 6, 2019
Merged
knative-prow-robot pushed a commit that referenced this pull request Jun 6, 2019
@knative-prow-robot
In memory subscribable status (fix fallout from #1330) (#1359)
a57f6a2 
* change to new status fields and codegen

* Populate the status subscribable properly

* use eventingclientset from Base, add unit test for status update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbrowning bbrowning Awaiting requested review from bbrowning

2 more reviewers

@Harwayne Harwayne Harwayne left review comments

@vaikas vaikas vaikas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Harwayne Harwayne

@nachocano nachocano

@vaikas vaikas

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@akashrv @knative-prow-robot @Harwayne @knative-metrics-robot @vaikas @nachocano @googlebot