add PingSource v1beta2 by eclipselu · Pull Request #4474 · knative/eventing

eclipselu · 2020-11-05T21:07:37Z

Proposed Changes

🎁 added PingSource v1beta2, it adds support for binary data encoded in base64.

Release Note

added PingSource v1beta2, it adds support for binary data encoded in base64.

Docs

codecov · 2020-11-05T21:14:21Z

Codecov Report

Merging #4474 (b43d52e) into master (fd20fbc) will increase coverage by 0.15%.
The diff coverage is 91.60%.

@@            Coverage Diff             @@
##           master    #4474      +/-   ##
==========================================
+ Coverage   81.27%   81.42%   +0.15%     
==========================================
  Files         284      290       +6     
  Lines        8017     8142     +125     
==========================================
+ Hits         6516     6630     +114     
- Misses       1113     1120       +7     
- Partials      388      392       +4

Impacted Files	Coverage Δ
pkg/adapter/mtping/adapter.go	`75.60% <ø> (ø)`
pkg/adapter/mtping/controller.go	`81.81% <ø> (ø)`
pkg/adapter/mtping/pingsource.go	`66.66% <ø> (ø)`
pkg/reconciler/pingsource/controller.go	`87.50% <ø> (ø)`
pkg/apis/sources/v1beta2/ping_lifecycle.go	`60.00% <60.00%> (ø)`
pkg/apis/sources/v1beta1/ping_conversion.go	`96.07% <96.07%> (-3.93%)`	⬇️
pkg/adapter/mtping/runner.go	`94.44% <100.00%> (+3.53%)`	⬆️
pkg/apis/sources/v1beta2/ping_conversion.go	`100.00% <100.00%> (ø)`
pkg/apis/sources/v1beta2/ping_defaults.go	`100.00% <100.00%> (ø)`
pkg/apis/sources/v1beta2/ping_types.go	`100.00% <100.00%> (ø)`
... and 10 more

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 fd20fbc...b43d52e. Read the comment docs.

eclipselu · 2020-11-09T17:45:18Z

/retest

matzew · 2020-11-12T13:06:56Z

@eclipselu this thing needs a rebase

eclipselu · 2020-11-12T18:52:59Z

@eclipselu this thing needs a rebase

Thanks for the reminder, this PR needs some more work. will do rebase

eclipselu · 2020-11-12T23:00:35Z

/retest

eclipselu · 2020-11-13T00:21:08Z

/retest

eclipselu · 2020-11-13T01:20:18Z

/retest

eclipselu · 2020-11-13T01:58:28Z

/test ?

knative-prow-robot · 2020-11-13T01:58:35Z

@eclipselu: The following commands are available to trigger jobs:

/test pull-knative-eventing-build-tests
/test pull-knative-eventing-unit-tests
/test pull-knative-eventing-integration-tests
/test pull-knative-eventing-conformance-tests
/test pull-knative-eventing-upgrade-tests
/test pull-knative-eventing-go-coverage

Use /test all to run all jobs.

Details

In response to this:

/test ?

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.

eclipselu · 2020-11-13T19:09:14Z

@eclipselu We can add the post-install job in a follow-up PR.

yeah, sure. and just to be clear, the post-install job will convert v1beta1 objects to v1beta2, and flip the storage version to v1beta2, we're introducing the post-install job in 0.21, not 0.20. only this pr will be released in 0.20

knative-metrics-robot · 2020-11-16T19:46:45Z

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/adapter/mtping/runner.go	94.6%	97.8%	3.2
pkg/apis/sources/v1beta1/ping_conversion.go	100.0%	97.7%	-2.3
pkg/apis/sources/v1beta2/ping_conversion.go	Do not exist	100.0%
pkg/apis/sources/v1beta2/ping_defaults.go	Do not exist	100.0%
pkg/apis/sources/v1beta2/ping_lifecycle.go	Do not exist	68.0%
pkg/apis/sources/v1beta2/ping_types.go	Do not exist	100.0%
pkg/apis/sources/v1beta2/ping_validation.go	Do not exist	100.0%
pkg/apis/sources/v1beta2/register.go	Do not exist	100.0%

nachocano · 2020-11-16T21:51:40Z

Thanks @eclipselu !!! Great work, I added a few nits in case you want to address them in this PR.

/approve
Leaving the lgtm to @lionelvillard

knative-prow-robot · 2020-11-16T21:51:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eclipselu, nachocano

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~~ [nachocano]

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

lionelvillard · 2020-11-17T15:40:16Z

+		}
+
+		// v1beta2 objects originally converted from v1beta1 is expected to have this annotation
+		v1beta1Spec, ok := annotations[V1B1SpecAnnotationKey]


IMO, the conversion should fail when the annotation is not present. Otherwise the spec looks incomplete.

the storage version is v1beta1, if we fail it here then when we create v1beta2 PingSource the annotation should also be specified, otherwise the creation will fail, for example:

$ kubectl create -n pingsource-example -f - <<EOF apiVersion: sources.knative.dev/v1beta2 kind: PingSource metadata: name: test-ping-source-v1beta2-x3 spec: schedule: "*/1 * * * *" contentType: "text/plain" data: "beta2 beta2 beta2" sink: ref: apiVersion: v1 kind: Service name: event-display EOF Error from server: error when creating "STDIN": conversion webhook for sources.knative.dev/v1beta2, Kind=PingSource failed: conversion failed to version v1beta1 for type [kind=PingSource group=sources.knative.dev version=v1beta2] - V1B1SpecAnnotationKey does not exist

Oops sorry I forgot the storage version is v1beta1. What feels strange is when getting the v1beta1 version of a v1beta2 instance, the spec is not what I would expect.

My example is this one:

apiVersion: sources.knative.dev/v1beta2 kind: PingSource metadata: name: hello-no-wrapper spec: schedule: "*/1 * * * *" data: "hello" sink: ref: apiVersion: v1 kind: Service name: event-display

$ kubectl get pingsources.v1beta1.sources.knative.dev hello-no-wrapper -oyaml ... metadata: annotations: pingsources.sources.knative.dev/v1beta2-spec: '{"sink":{"ref":{"kind":"Service","name":"event-display","apiVersion":"v1"}},"schedule":"*/1 * * * *","data":"hello"}' spec: schedule: '* * * * *' sink: {} ...

Not a biggie. I don't think getting a previous version is common.

yes, this goes back to our original discussion on round-tripping, there's no deterministic way to know the expected v1beta1 spec by only looking at v1beta2 spec.

so here v1beta1 just serves as an envelope for v1beta2 spec.

lionelvillard · 2020-11-17T17:12:30Z

@eclipselu can you create 2 issues, one to update the documentation and one for adding a post-install job upgrading the storage version to v1beta2?

/lgtm

eclipselu · 2020-11-17T19:20:09Z

@nachocano this PR got auto-merged without the nits fix, please take a look here: #4537 thanks!

lionelvillard · 2020-11-17T19:45:26Z

sorry about that, I should have put a hold.

nachocano · 2020-11-17T19:47:09Z

all good, glad this got in!

…

On Tue, Nov 17, 2020 at 11:45 AM Lionel Villard ***@***.***> wrote: sorry about that, I should have put a hold. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4474 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABD65DE2D2EA5T3YJWK75XLSQLHGJANCNFSM4TL3YKRQ> .

eclipselu · 2020-11-17T22:55:07Z

sorry about that, I should have put a hold.

no worries, the PR was merged too.

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2020

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 5, 2020

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Nov 5, 2020

knative-prow-robot requested review from slinkydeveloper, steuhs and yt3liu November 5, 2020 21:07

lionelvillard reviewed Nov 6, 2020

View reviewed changes

Comment thread config/core/resources/pingsource.yaml Outdated

lionelvillard reviewed Nov 6, 2020

View reviewed changes

Comment thread config/core/resources/pingsource.yaml Outdated

lionelvillard reviewed Nov 6, 2020

View reviewed changes

Comment thread config/core/resources/pingsource.yaml Outdated

lionelvillard reviewed Nov 6, 2020

View reviewed changes

Comment thread pkg/adapter/mtping/pingsource_test.go

lionelvillard reviewed Nov 6, 2020

View reviewed changes

Comment thread pkg/adapter/mtping/runner.go Outdated

lionelvillard reviewed Nov 6, 2020

View reviewed changes

Comment thread pkg/apis/sources/v1beta2/ping_validation.go

eclipselu changed the title ~~[WIP] add PingSource v1beta2~~ add PingSource v1beta2 Nov 9, 2020

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 Nov 9, 2020

eclipselu changed the title ~~add PingSource v1beta2~~ [WIP] add PingSource v1beta2 Nov 10, 2020

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 10, 2020

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2020

eclipselu changed the title ~~[WIP] add PingSource v1beta2~~ add PingSource v1beta2 Nov 13, 2020

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 Nov 13, 2020

eclipselu added 2 commits November 13, 2020 12:57

fix nits

c92f26f

validate contentType in pkg/adapter/mtping/runner_test.go

9ee99e5

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2020

default to pass []byte(data) to event.SetData

b43d52e