kebabfy broker channel spec and pingsource opts

[odacremolbap]

Fixes #5789

Following the same path as PRs at serving, this one is setting config element names that were previously camel cased to kebab case in a backwards compatible way.

---

Note: I tried to also change camelcased annotations/labels, found this that could probably need to be:

actual	desired
eventing.knative.dev/brokerRole	broker-role
eventing.knative.dev/broker.class	broker-class
eventing.knative.dev/sourceName	source-name
eventing.knative.dev/brokerEverything	broker-everything

But the first one is being used at Deployment selectors, changing it would mean re-deploying broker's controllers and losing data. I am not sure if it is worth it since it wasn't part of the issue I was tackling, hence de-scoping.

Proposed Changes

• 🧹 Update or clean up current behavior

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior
• Docs PR for any user-facing impact
• Spec PR for any new API feature
• Conformance test for any change to the spec

Release Note

The Channel template at the ConfigMap that Brokers use to declare the underlying channel must be located under the `channel-template-spec` element. The previous `channelTemplateSpec` element has been deprecated.

PingSource's ConfigMap element for maximum size has been redefined as `data-max-size`. The previous  `dataMaxSize` element has been deprecated.

Docs

📖 knative/docs#4453

[knative-prow-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[codecov]

Codecov Report

Merging #5875 (6da07d3) into main (aef6803) will increase coverage by 0.10%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##             main    #5875      +/-   ##
==========================================
+ Coverage   81.96%   82.07%   +0.10%     
==========================================
  Files         220      220              
  Lines        7458     7458              
==========================================
+ Hits         6113     6121       +8     
+ Misses        918      909       -9     
- Partials      427      428       +1     

Impacted Files	Coverage Δ
pkg/reconciler/broker/config.go	76.47% <75.00%> (-10.20%)	⬇️
pkg/apis/sources/config/ping_defaults.go	87.50% <100.00%> (+31.94%)	⬆️
pkg/reconciler/broker/trigger/trigger.go	83.33% <0.00%> (+2.56%)	⬆️

---

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 aef6803...6da07d3. Read the comment docs.

[odacremolbap]

/assign @lberk

[odacremolbap]

/test all

[odacremolbap]

/test pull-knative-eventing-upgrade-tests

[matzew]

/hold

Do we need to have some post-install job for this ? Or any plan of migration?

[odacremolbap]

/hold

Do we need to have some post-install job for this ? Or any plan of migration?

@matzew
Not needed because previous names still work.
tbh, I am not sure why serving went the compatibility way instead of adding a migration job, I just followed through.

[lionelvillard]

@odacremolbap can you keep the unit tests with the old keys, to make sure we are not breaking anything?

[odacremolbap]

@lionelvillard previous names are being maintained at the configuration tests along with the new values:

eventing/pkg/reconciler/broker/config_test.go

Line 105 in 75dba17

"channelTemplateSpec": `

eventing/pkg/apis/sources/config/ping_defaults_test.go

Line 97 in 75dba17

dataMaxSize: 4096

I didn't duplicate tests for both labels everywhere because it sounded a bit repetitive, but maybe I can switch and use the old naming at most places and the new one only at the configuration tests. The reason I decided in the opposite direction was to educate by example.

[matzew]

On Tue 9. Nov 2021 at 17:58, Pablo Mercado ***@***.***> wrote: /hold Do we need to have some post-install job for this ? Or any plan of migration? @matzew <https://github.com/matzew> Not needed because previous names still work. tbh, I am not sure why serving went the compatibility way instead of adding a migration job <knative/serving#7300>, I just followed through.

Ah. ok! I am just double checking we are not breaking anyone. Especially those using the operator w/ auto-upgrades. Thx for pointing that out! —

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5875 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGPTRX3GLNTC2RSJJFQDLULFHL5ANCNFSM5HMATWOQ> .

-- Sent from Gmail Mobile

[odacremolbap]

Added more tests using the previous name, scattered at each affected location and including messages that warns about future deprectaion. See: 6da07d3

@matzew @lionelvillard

[knative-metrics-robot]

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/sources/config/ping_defaults.go	55.6%	92.3%	36.8
pkg/reconciler/broker/config.go	93.3%	85.7%	-7.6

[odacremolbap]

/retest

[pierDipi]

/lgtm

[lionelvillard]

/approve

@matzew If you are ok with this, can you unhold?

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lionelvillard, odacremolbap

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 [lionelvillard]

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

[odacremolbap]

@matzew is there something pending here or can this be unholded?

[matzew]

I will unhold once we have cut the 1.1 release, if that's OK

[csantanapr]

bump @matzew @odacremolbap

[matzew]

/unhold

On Thu 16. Dec 2021 at 20:19, Carlos Santana ***@***.***> wrote: bump @matzew <https://github.com/matzew> @odacremolbap <https://github.com/odacremolbap> — Reply to this email directly, view it on GitHub <#5875 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGPTRURMSEYXHBPSBVYALURI3TLANCNFSM5HMATWOQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Sent from Gmail Mobile