Cleanup channel template internal types #4749

slinkydeveloper · 2021-01-15T13:28:10Z

While working on #4652, we found out that we have these internal types, necessary to implement some duck typing magic, that are implementation details that shouldn't be exposed and shouldn't be versioned. This PR cleanup that code and adds a single entrypoint to create from the various ducks a physical channel, removing the various duplicated code in each controller.

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Proposed Changes

🧹 Cleanup channel internal templates and moved logic to create physical channel in eventing/pkg/duck

Release Note

Cleanup channel duck types internals

devguyio · 2021-01-15T13:30:26Z

/assign

vaikas · 2021-01-15T13:52:12Z

I'm loving the red lines. Could you add more comprehensive UT so we get more coverage?

codecov · 2021-01-15T14:09:04Z

Codecov Report

Merging #4749 (f05842a) into master (8e0e286) will increase coverage by 0.14%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##           master    #4749      +/-   ##
==========================================
+ Coverage   81.11%   81.25%   +0.14%     
==========================================
  Files         291      291              
  Lines        8239     8281      +42     
==========================================
+ Hits         6683     6729      +46     
+ Misses       1156     1143      -13     
- Partials      400      409       +9

Impacted Files	Coverage Δ
pkg/reconciler/mtbroker/resources/channel.go	`100.00% <ø> (+8.33%)`	⬆️
pkg/duck/channel.go	`54.54% <54.54%> (ø)`
pkg/reconciler/channel/channel.go	`71.60% <100.00%> (+4.46%)`	⬆️
pkg/reconciler/mtbroker/broker.go	`80.46% <100.00%> (+2.02%)`	⬆️
pkg/reconciler/parallel/parallel.go	`58.82% <100.00%> (+3.77%)`	⬆️
pkg/reconciler/sequence/sequence.go	`71.00% <100.00%> (+3.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e0e286...ce87ac2. Read the comment docs.

devguyio

Just a comment about maybe trying to document the channel duck package more. +1 on more UT coverage. Other than that, lgtm.

devguyio · 2021-01-15T13:55:31Z

pkg/duck/channel.go

+
+	duckv1 "knative.dev/eventing/pkg/apis/duck/v1"
+)
+


Well, seeing this is very essential package in eventing pkg to be used in gazillion of other components, I think it would be great to add a more detailed generic comment explaining the context here. Example points to cover:

What is the physical channel?

How do the different structs relate to the Channel type.

Maybe with an example?

I see your point, let me add some additional docs to NewPhysicalChannel so it explains why this thing is important

slinkydeveloper · 2021-01-15T15:08:18Z

@vaikas now the tests should cover a good amount of code
@devguyio added a big godoc block that should answer your questions

slinkydeveloper · 2021-01-15T15:28:05Z

/retest

tommyreddad · 2021-01-15T16:15:27Z

pkg/reconciler/mtbroker/resources/channel_test.go

-				},
-			},
-		},
-		"KafkaChannel": {


It seems okay to remove some of these tests, especially the KafkaChannel test, as it should be the responsibility of eventing-kafka. But there seem to be other things being thrown out by removing this test entirely, e.g., the BrokerChannelName check. Any reason for this?

The reason is that the tested function here doesn't exist anymore. There is still a test for BrokerChannelName

I see. And the OwnerReferences test done by assertSoleOwner?

assertSoleOwner was an assert executed by the deleted test, and note that now the OwnerReferences is set by the caller of NewPhysicalChannel

slinkydeveloper · 2021-01-15T16:23:51Z

/retest

…venting/pkg/duck module Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-metrics-robot · 2021-01-18T08:05:30Z

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/duck/channel.go	Do not exist	76.7%
pkg/reconciler/mtbroker/resources/channel.go	90.0%	100.0%	10.0

vaikas · 2021-01-18T08:53:12Z

/lgtm
/approve
/hold
since @devguyio had some comments as well, if you're ok with the changes requested, just remove the hold.

devguyio

great work!
/lgtm
/unhold

knative-prow-robot · 2021-01-18T09:31:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devguyio, slinkydeveloper, vaikas

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~~ [slinkydeveloper,vaikas]

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

slinkydeveloper · 2021-01-18T10:52:56Z

/retest
some flakyness not related to this pr

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 15, 2021

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release labels Jan 15, 2021

knative-prow-robot requested review from aliok and yt3liu January 15, 2021 13:28

knative-prow-robot assigned devguyio Jan 15, 2021

devguyio self-requested a review January 15, 2021 13:30

devguyio suggested changes Jan 15, 2021

View reviewed changes

tommyreddad reviewed Jan 15, 2021

View reviewed changes

slinkydeveloper added 6 commits January 18, 2021 08:59

Cleanup channel template types and encapsulate the functionality in e…

de7f343

…venting/pkg/duck module Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Missing copyright

8cba5db

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Missing fix

62ed3d9

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Missing fix

47c6a94

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Improved tests

9b1b7cb

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Doc'ed

ce87ac2

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper force-pushed the cleanup_template_duck_types branch from 226d206 to ce87ac2 Compare January 18, 2021 08:00

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2021

knative-prow-robot assigned vaikas Jan 18, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2021

devguyio approved these changes Jan 18, 2021

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2021

knative-prow-robot merged commit d395f6f into knative:master Jan 18, 2021

slinkydeveloper deleted the cleanup_template_duck_types branch January 18, 2021 11:33

slinkydeveloper mentioned this pull request Jan 25, 2021

Cleanup the api packages to remove unused duck templates #4728

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup channel template internal types #4749

Cleanup channel template internal types #4749

slinkydeveloper commented Jan 15, 2021

devguyio commented Jan 15, 2021

vaikas commented Jan 15, 2021

codecov bot commented Jan 15, 2021 •

edited

Loading

devguyio left a comment

devguyio Jan 15, 2021

slinkydeveloper Jan 15, 2021

slinkydeveloper commented Jan 15, 2021

slinkydeveloper commented Jan 15, 2021

tommyreddad Jan 15, 2021

slinkydeveloper Jan 15, 2021

tommyreddad Jan 15, 2021

slinkydeveloper Jan 18, 2021

slinkydeveloper commented Jan 15, 2021

knative-metrics-robot commented Jan 18, 2021

vaikas commented Jan 18, 2021

devguyio left a comment

knative-prow-robot commented Jan 18, 2021

slinkydeveloper commented Jan 18, 2021

Cleanup channel template internal types #4749

Cleanup channel template internal types #4749

Conversation

slinkydeveloper commented Jan 15, 2021

Proposed Changes

devguyio commented Jan 15, 2021

vaikas commented Jan 15, 2021

codecov bot commented Jan 15, 2021 • edited Loading

Codecov Report

devguyio left a comment

Choose a reason for hiding this comment

devguyio Jan 15, 2021

Choose a reason for hiding this comment

slinkydeveloper Jan 15, 2021

Choose a reason for hiding this comment

slinkydeveloper commented Jan 15, 2021

slinkydeveloper commented Jan 15, 2021

tommyreddad Jan 15, 2021

Choose a reason for hiding this comment

slinkydeveloper Jan 15, 2021

Choose a reason for hiding this comment

tommyreddad Jan 15, 2021

Choose a reason for hiding this comment

slinkydeveloper Jan 18, 2021

Choose a reason for hiding this comment

slinkydeveloper commented Jan 15, 2021

knative-metrics-robot commented Jan 18, 2021

vaikas commented Jan 18, 2021

devguyio left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 18, 2021

slinkydeveloper commented Jan 18, 2021

codecov bot commented Jan 15, 2021 •

edited

Loading