Skip to content

Improve error handling at conversion of Subscribable subtypes#3476

Merged
knative-prow-robot merged 3 commits into
knative:masterfrom
aliok:duck_conv_05_error_handling
Jul 7, 2020
Merged

Improve error handling at conversion of Subscribable subtypes#3476
knative-prow-robot merged 3 commits into
knative:masterfrom
aliok:duck_conv_05_error_handling

Conversation

@aliok
Copy link
Copy Markdown
Member

@aliok aliok commented Jul 3, 2020 •
edited
Loading

Part of #3474

  • Adds error handling for Subscribable related subtype conversions

@aliok aliok changed the title Improve error handling at conversion of Subscribable subtypes [WIP] Improve error handling at conversion of Subscribable subtypes Jul 3, 2020
@knative-prow-robot knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 3, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 3, 2020
@aliok aliok mentioned this pull request Jul 3, 2020
Merged
@aliok
Copy link
Copy Markdown
Member Author

aliok commented Jul 3, 2020

/retest

@knative-test-reporter-robot
Copy link
Copy Markdown

knative-test-reporter-robot commented Jul 3, 2020

The following jobs failed:

Test name Triggers Retries
pull-knative-eventing-integration-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-integration-tests:

test/e2e.TestEventTransformationForSubscription
test/e2e.TestSingleStructuredEventForChannelV1
test/e2e.TestSequenceV1
test/e2e.TestEventTransformationForSubscription/Channel-messaging.knative.dev/v1
test/e2e.TestSingleStructuredEventForChannelV1/Channel-messaging.knative.dev/v1
test/e2e.TestSequenceV1/Channel-messaging.knative.dev/v1
test/conformance.TestChannelDataPlaneFailure
test/conformance.TestChannelDataPlaneFailure/Channel-messaging.knative.dev/v1

and 1 more.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2020
@aliok aliok force-pushed the duck_conv_05_error_handling branch from 977940c to 6c49c7d Compare July 6, 2020 06:41
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 6, 2020
@aliok aliok force-pushed the duck_conv_05_error_handling branch from 6c49c7d to 36fa11c Compare July 6, 2020 21:31
@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 6, 2020
@aliok aliok changed the title [WIP] Improve error handling at conversion of Subscribable subtypes Improve error handling at conversion of Subscribable subtypes Jul 6, 2020
@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 Jul 6, 2020
@aliok
Copy link
Copy Markdown
Member Author

aliok commented Jul 6, 2020

cc @nlopezgi @vaikas

aliok
aliok commented Jul 6, 2020
View reviewed changes
Comment thread pkg/apis/duck/v1alpha1/subscribable_types_conversion.go
@@ -32,11 +32,16 @@ func (source *SubscribableType) ConvertTo(ctx context.Context, obj apis.Converti
switch sink := obj.(type) {
case *duckv1beta1.Subscribable:
Copy link
Copy Markdown
Member Author

@aliok aliok Jul 6, 2020

Choose a reason for hiding this comment

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

I am gonna finally add v1alpha1<>v1 conversions after this PR. Please bear with me.

@n3wscott
Copy link
Copy Markdown
Contributor

n3wscott commented Jul 6, 2020

Could you please add a grip of tests for this too? We have found this type of code quite tricky to get right without a lot of tests.

lionelvillard
lionelvillard reviewed Jul 6, 2020
View reviewed changes
Comment thread pkg/apis/duck/v1alpha1/subscribable_types_conversion.go Outdated
lionelvillard
lionelvillard reviewed Jul 6, 2020
View reviewed changes
Comment thread pkg/apis/duck/v1alpha1/subscribable_types_conversion.go Outdated
@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Jul 7, 2020

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

File Old Coverage New Coverage Delta
pkg/apis/duck/v1alpha1/subscribable_types_conversion.go 98.4% 91.2% -7.2
pkg/apis/duck/v1beta1/subscribable_types_conversion.go 94.9% 89.4% -5.5

@aliok
Copy link
Copy Markdown
Member Author

aliok commented Jul 7, 2020

@n3wscott
I added another test case, but the coverage can go as high as this.
What I did in this PR is to handle the errors returned by the subtype (e.g. SubscriberSpec etc.) conversions. Most of those conversions do not return any actual errors today, thus practically there's no behavior change. But, IMO, it is better to handle the potential returned errors as there will be more code in those conversions in the future.

I would be happy to change any part of the code if anybody has any suggestion.

@nlopezgi
Copy link
Copy Markdown
Contributor

nlopezgi commented Jul 7, 2020

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2020
@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Jul 7, 2020

/lgtm
/approve

@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jul 7, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, vaikas

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 Jul 7, 2020
@knative-prow-robot knative-prow-robot merged commit 5de5d77 into knative:master Jul 7, 2020
@aliok aliok deleted the duck_conv_05_error_handling branch April 28, 2021 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grantr grantr Awaiting requested review from grantr

1 more reviewer

@lionelvillard lionelvillard lionelvillard left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@nlopezgi nlopezgi

@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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@aliok @knative-test-reporter-robot @n3wscott @knative-metrics-robot @nlopezgi @vaikas @knative-prow-robot @lionelvillard @googlebot