Skip to content

Updated Natss channel to set subscribable status#1373

Merged
knative-prow-robot merged 7 commits into
knative:masterfrom
akashrv:natss1189
Jun 11, 2019
Merged

Updated Natss channel to set subscribable status#1373
knative-prow-robot merged 7 commits into
knative:masterfrom
akashrv:natss1189

Conversation

@akashrv
Copy link
Copy Markdown
Contributor

@akashrv akashrv commented Jun 7, 2019

reference #1189

Proposed Changes

  • Natss dispatcher controller now sets the subscribable status and updates the channel status

Release Note

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 7, 2019
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2019
@akashrv
Copy link
Copy Markdown
Contributor Author

akashrv commented Jun 7, 2019

/assign @vaikas-google @nachocano @Harwayne

nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/dispatcher/dispatcher.go Outdated
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/reconciler/dispatcher/natsschannel.go Outdated
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/reconciler/dispatcher/natsschannel.go Outdated
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/reconciler/dispatcher/natsschannel.go
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/reconciler/dispatcher/natsschannel.go Outdated
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/reconciler/dispatcher/natsschannel.go Outdated
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/reconciler/dispatcher/natsschannel.go Outdated
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/reconciler/dispatcher/natsschannel.go Outdated
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/dispatcher/dispatcher.go Outdated
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/dispatcher/dispatcher.go Outdated
nachocano
nachocano reviewed Jun 7, 2019
View reviewed changes
Comment thread contrib/natss/pkg/dispatcher/dispatcher.go
@akashrv
Copy link
Copy Markdown
Contributor Author

akashrv commented Jun 8, 2019

/hold

@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 8, 2019
nachocano
nachocano reviewed Jun 10, 2019
View reviewed changes
Comment thread contrib/natss/pkg/dispatcher/dispatcher.go
}

func (s *SubscriptionsSupervisor) UpdateSubscriptions(channel *eventingv1alpha1.Channel, isFinalizer bool) error {
// UpdateSubscriptions creates/deletes the natss subscriptions based on channel.Spec.Subscribable.Subscribers
Copy link
Copy Markdown
Contributor

@nachocano nachocano Jun 10, 2019

Choose a reason for hiding this comment

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

it would be nice to describe what it returns as well, now that it returns a map of failed subscriptions?

nachocano
nachocano reviewed Jun 10, 2019
View reviewed changes
Comment thread contrib/natss/pkg/reconciler/dispatcher/natsschannel.go Outdated
}
natssChannel.Status.SubscribableStatus = r.createSubscribableStatus(natssChannel.Spec.Subscribable, failedSubscriptions)
if len(failedSubscriptions) > 0 {
logging.FromContext(ctx).Error("Some natss subscriptions failed to subscribe")
Copy link
Copy Markdown
Contributor

@nachocano nachocano Jun 10, 2019

Choose a reason for hiding this comment

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

maybe also log the len(failedSubscriptions)?
I'm fine if you don't, but the current error message doesn't give enough info to debug...

@nachocano
Copy link
Copy Markdown
Contributor

nachocano commented Jun 10, 2019

/lgtm

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

knative-metrics-robot commented Jun 11, 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/natss/pkg/dispatcher/dispatcher.go 57.6% 55.9% -1.7

@nachocano
Copy link
Copy Markdown
Contributor

nachocano commented Jun 11, 2019

/lgtm

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

nachocano commented Jun 11, 2019

Let's get this in, so that we can enable the e2e test, and keep on improving it.

@nachocano
Copy link
Copy Markdown
Contributor

nachocano commented Jun 11, 2019

/hold cancel

/cc @vaikas-google for approval

@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 11, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jun 11, 2019

@nachocano: GitHub didn't allow me to request PR reviews from the following users: for, approval.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/hold cancel

/cc @vaikas-google for approval

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.

@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Jun 11, 2019

/approve

@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jun 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akashrv, vaikas-google

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 11, 2019
@knative-prow-robot knative-prow-robot merged commit 705d97d into knative:master Jun 11, 2019
@nachocano nachocano mentioned this pull request Oct 29, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devguyio devguyio Awaiting requested review from devguyio

@grantr grantr Awaiting requested review from grantr

@vaikas vaikas Awaiting requested review from vaikas

1 more reviewer

@nachocano nachocano nachocano 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

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