fix(PingSource): remove annotation based conversion logic by eclipselu · Pull Request #5153 · knative/eventing

eclipselu · 2021-03-25T18:59:27Z

Proposed Changes

🧹 v1beta1 will not always be roundtrip-able (in the case that jsonData cannot be marshalled into json, e.g. jsonData="hello"), but the cloudevent on the wire stays the same.
🐛 duckv1.SourceSpec, TimeZone, Schedule will be populated when called by v1beta1 clients
🧹 object with v1beta2-specific features (dataBase64, contentType beyond application/json) will not populate jsonData field when converted to v1beta1.

Release Note

:bug: duckv1.SourceSpec, TimeZone, Schedule will be populated when called by v1beta1/v1alpha2 clients

Docs

codecov · 2021-03-25T19:06:25Z

Codecov Report

Merging #5153 (92bb7ea) into main (d3e856a) will increase coverage by 0.59%.
The diff coverage is 95.00%.

❗ Current head 92bb7ea differs from pull request most recent head df8dfd8. Consider uploading reports for the commit df8dfd8 to get more accurate results

@@            Coverage Diff             @@
##             main    #5153      +/-   ##
==========================================
+ Coverage   83.41%   84.01%   +0.59%     
==========================================
  Files         283      283              
  Lines        7955     7933      -22     
==========================================
+ Hits         6636     6665      +29     
+ Misses        947      897      -50     
+ Partials      372      371       -1

Impacted Files	Coverage Δ
pkg/apis/sources/v1beta1/ping_conversion.go	`95.12% <95.00%> (+8.63%)`	⬆️
pkg/adapter/v2/main.go	`60.86% <0.00%> (+2.73%)`	⬆️
pkg/kncloudevents/utils.go	`69.23% <0.00%> (+69.23%)`	⬆️
pkg/kncloudevents/message_receiver.go	`90.69% <0.00%> (+90.69%)`	⬆️

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 d3e856a...df8dfd8. Read the comment docs.

eclipselu · 2021-03-25T19:48:48Z

/test pull-knative-eventing-reconciler-tests

eclipselu · 2021-03-25T20:32:19Z

/test pull-knative-eventing-reconciler-tests

lberk · 2021-03-29T16:57:07Z

it would be great to get this backported to v0.22 & v0.21 given #5137

- v1beta1 will not guaranteed to be roundtrip-able (in the case that jsonData cannot be marshalled into json, e.g. jsonData="hello"), but the cloudevent on the wire stays the same. - duckv1.SourceSpec, TimeZone, Schedule will be populated when called by v1beta1 clients - object with v1beta2-specific features (dataBase64, contentType beyond `application/json`) will not populate jsonData field when converted to v1beta1.

eclipselu · 2021-03-31T01:54:31Z

/test pull-knative-eventing-reconciler-tests

lionelvillard · 2021-03-31T15:24:09Z

BTW, since there is no need for the annotations anymore, what about removing them when running the post-install job? (See WIP: #5008)

eclipselu · 2021-03-31T15:28:48Z

BTW, since there is no need for the annotations anymore, what about removing them when running the post-install job? (See WIP: #5008)

yes we can do it.

lionelvillard · 2021-04-02T18:10:05Z

@eclipselu Is there anything else to do here?

eclipselu · 2021-04-02T19:10:59Z

@eclipselu Is there anything else to do here?

The only question is that if we return an error like discussed, while it makes sense in some way, it still does not fix the original issue (duckv1.SourceSpec isn't populated for non-storage versions).

The most impacted is kn, which uses v1alpha2 source client, list/describe will both fail. Specifically for kn source ping list, when there're one v1beta1 PingSource and one v1beta2 PingSource, it will still fail.

eclipselu · 2021-04-06T14:46:55Z

/test pull-knative-eventing-unit-tests

eclipselu · 2021-04-06T15:54:45Z

/test pull-knative-eventing-reconciler-tests

knative-metrics-robot · 2021-04-06T21:09:40Z

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/v1beta1/ping_conversion.go	92.4%	95.8%	3.4

lionelvillard · 2021-04-12T19:35:54Z

Much better! Thanks @eclipselu

/lgtm
/approve

knative-prow-robot · 2021-04-12T19:36:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lionelvillard

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:

~~OWNERS~~ [lionelvillard]

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

eclipselu · 2021-04-12T20:03:55Z

/test pull-knative-eventing-reconciler-tests

eclipselu · 2021-04-12T20:04:46Z

Will be back-ported to 0.22 and 0.21.

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 25, 2021

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 25, 2021

eclipselu force-pushed the fix branch from e23cc0f to dc5cc4d Compare March 25, 2021 19:10

lionelvillard reviewed Mar 29, 2021

View reviewed changes

Comment thread pkg/apis/sources/v1beta1/ping_conversion.go Outdated

eclipselu force-pushed the fix branch from dc5cc4d to df8dfd8 Compare March 31, 2021 01:27

eclipselu changed the title ~~fix(PingSource): populate duckv1.SourceSpec, schedule and timezone when converting from v1beta2 to v1beta1~~ fix(PingSource): remove annotation based conversion logic Mar 31, 2021

eclipselu commented Mar 31, 2021

View reviewed changes

Comment thread pkg/apis/sources/v1beta1/ping_conversion.go

eclipselu force-pushed the fix branch from 92bb7ea to df8dfd8 Compare April 6, 2021 21:03

knative-prow-robot assigned lionelvillard Apr 12, 2021

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

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2021

eclipselu mentioned this pull request Apr 12, 2021

Remove PingSource spec annotations #5232

Closed

knative-prow-robot merged commit 515136c into knative:main Apr 12, 2021

This was referenced Apr 12, 2021

fix(PingSource): remove annotation based conversion logic #5233

Merged

fix(PingSource): remove annotation based conversion logic #5234

Merged

Conversation

eclipselu commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

codecov Bot commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eclipselu commented Mar 25, 2021

Uh oh!

eclipselu commented Mar 25, 2021

Uh oh!

Uh oh!

lberk commented Mar 29, 2021

Uh oh!

eclipselu commented Mar 31, 2021

Uh oh!

Uh oh!

lionelvillard commented Mar 31, 2021

Uh oh!

eclipselu commented Mar 31, 2021

Uh oh!

lionelvillard commented Apr 2, 2021

Uh oh!

eclipselu commented Apr 2, 2021

Uh oh!

eclipselu commented Apr 6, 2021

Uh oh!

eclipselu commented Apr 6, 2021

Uh oh!

knative-metrics-robot commented Apr 6, 2021

Uh oh!

lionelvillard commented Apr 12, 2021

Uh oh!

knative-prow-robot commented Apr 12, 2021

Uh oh!

eclipselu commented Apr 12, 2021

Uh oh!

eclipselu commented Apr 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eclipselu commented Mar 25, 2021 •

edited

Loading

codecov Bot commented Mar 25, 2021 •

edited

Loading