Update spec.md with v1 API by AlexandraRoatis · Pull Request #4594 · knative/eventing

AlexandraRoatis · 2020-11-25T16:31:36Z

The Object Model descriptions are out of date.

Also removed a duplicate spec file.

Proposed Changes

update/add object descriptions
update groups to eventing.knative.dev/v1
use consistent types: String -> string; int64 -> int
replace ObjectReference with KReference
add Requirement and Defaults columns
add annotations to Status descriptions
add missing Status conditions
add section on how conditions are used based on serving spec
detail the Addressable interface
update/remove Shared Schema Objects

Release Note

Docs

Detailed changes: - update/add object descriptions - update groups to eventing.knative.dev/v1 - use consistent types: String -> string; int64 -> int - replace ObjectReference with KReference - add Requirement and Defaults columns - add annotations to Status descriptions - add missing Status conditions - add section on how conditions are used based on serving spec - detail the Addressable interface - update/remove Shared Schema Objects

AlexandraRoatis · 2020-11-25T16:35:01Z

/assign @vaikas
/assign @grantr

aliok · 2020-11-30T09:22:08Z

/retest

Thanks for updating this doc.

Let's see if unit tests still fail.

vaikas

Thanks so much for working on this!

vaikas · 2020-12-08T22:30:18Z

+| `conditions`         | [`[]apis.Condition`](#apis.condition)       | Optional    | Channel conditions. The latest available observations of the resource's current state.                                 |             |
+| `annotations`        | `map[string]string`                         | Optional    | Fields to save additional state as well as convey more information to the user.                                        |             |
+| `address`            | [`duckv1.Addressable`](#duckv1.addressable) | Required    | Address of the endpoint (as an URI) for getting events delivered into the channel.                                     |             |
+| `subscribers`        | [`[]SubscriberStatus`](#subscriberstatus)   | Required    | The list of statuses for each of the channel's subscribers.                                                            |             |


I think you can point to a eventingduckv1.ChannelableStatus to remove some of these fields?

I expanded the inherited types throughout the spec and only referenced and described the ones that are used as field types. Referencing ChannelableStatus would break that pattern. Given this information, do you still prefer using ChannelableStatus here?

codecov · 2020-12-21T22:16:59Z

Codecov Report

Merging #4594 (11687b5) into master (b1882d6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4594   +/-   ##
=======================================
  Coverage   81.05%   81.05%           
=======================================
  Files         291      291           
  Lines        8220     8220           
=======================================
  Hits         6663     6663           
  Misses       1156     1156           
  Partials      401      401

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 b1882d6...11687b5. Read the comment docs.

zhongduo · 2021-01-05T17:39:35Z

+| `deadLetterSink` | [`duckv1.Destination`](#duckv1.Destination) | Optional    | The sink receiving event that could not be sent to a `Destination`.                                                                                               |             |
+| `retry`          | `int`                                       | Optional    | The minimum number of retries the sender should attempt when sending an event before moving it to the dead letter sink (when specified) or discarded (otherwise). |             |
+| `backoffPolicy`  | `string`                                    | Optional    | The retry backoff policy (`linear` or `exponential`).                                                                                                             |             |
+| `backoffDelay`   | `string`                                    | Optional    | For linear policy, backoff delay is backoffDelay\*<numberOfRetries>. For exponential policy, backoff delay is backoffDelay\*2^<numberOfRetries>.                  |             |


This will have the same problem as in PR #4690. Can you please make sure this PR will be applied here too?

tayarani · 2021-01-11T17:57:53Z

@AlexandraRoatis some tests are failing. Looks like vendoring issues.

AlexandraRoatis · 2021-01-11T18:02:06Z

@tayarani I expect that needs to be fixed externally, since it's not related to this change.

tayarani · 2021-01-11T18:13:13Z

Yeah, I would assume another rebase on master would fix it. I wasn't recommending you run any update scripts.

vaikas · 2021-01-12T07:44:19Z

+# Implementation Helper
+
+This document supplements the official [spec](spec.md) with useful
+implementation details.


If this only pertains to the ChannelBased Broker, then I think we should state that up front so it's clear which implementation details we're talking about. Otherwise the conditions, for example L40 should be matched with the spec.

vaikas · 2021-01-12T07:46:19Z

Thank you so much for continued work on this. I personally think that this is so close that I'd be happy to get this merged and iterate with smaller changes as necessary.
/lgtm
/approve
/hold
Just a hold if other folks reviewing have blockers before merging. Failing tests are unrelated.

AlexandraRoatis · 2021-01-12T22:39:35Z

Unfortunately, merging the master branch in seems to have not helped with the failing tests.

vaikas · 2021-01-13T11:22:42Z

Ok, natss / rabbitmq were fixed, re-ran the jobs and greenness is all around us, woot woot!. I'm going to merge this and we can make more changes in a followup PRs.

I created an issue for implementation spec file that we can tackle, if there are others that need followup let's create an issue for those.
#4732

Thanks much for doing this!!!
/lgtm
/approve

knative-prow-robot · 2021-01-13T11:22:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexandraRoatis, 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:

~~OWNERS~~ [vaikas]

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

vaikas · 2021-01-13T11:28:22Z

/unhold

AlexandraRoatis added 2 commits November 25, 2020 11:20

Remove spec_v1alpha1.md

09a5253

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

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 25, 2020

knative-prow-robot requested review from Harwayne and tommyreddad November 25, 2020 16:31

knative-prow-robot assigned grantr and vaikas Nov 25, 2020

AlexandraRoatis changed the title ~~Update spec.md with v1 API~~ [WIP] Update spec.md with v1 API Nov 25, 2020

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 25, 2020

zhongduo reviewed Dec 8, 2020

View reviewed changes

Comment thread docs/spec/spec.md

Comment thread docs/spec/spec.md Outdated

Comment thread docs/spec/spec.md Outdated

Comment thread docs/spec/spec.md Outdated

vaikas reviewed Dec 8, 2020

View reviewed changes

Move events and subconditions to a separate helper.md document

5a1492e

Update spec.md according to review comments

e41965c

zhongduo reviewed Jan 5, 2021

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 5, 2021

Merge branch 'master' into specmd

53102e1

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

AlexandraRoatis changed the title ~~[WIP] Update spec.md with v1 API~~ Update spec.md with v1 API Jan 8, 2021

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 Jan 8, 2021

vaikas reviewed Jan 12, 2021

View reviewed changes

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jan 12, 2021

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

Merge branch 'master' into specmd

11687b5

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

vaikas mentioned this pull request Jan 13, 2021

If this only pertains to the ChannelBased Broker, then I think we should state that up front so it's clear which implementation details we're talking about. Otherwise the conditions, for example L40 should be matched with the spec. #4732

Closed

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

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 13, 2021

knative-prow-robot merged commit 47d9937 into knative:master Jan 13, 2021

slinkydeveloper mentioned this pull request Jan 14, 2021

Fix spec #4515 #4654

Merged

AlexandraRoatis mentioned this pull request Jan 15, 2021

Move the produced events details out of spec.md #4596

Closed

Conversation

AlexandraRoatis commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

AlexandraRoatis commented Nov 25, 2020

Uh oh!

aliok commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vaikas Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

AlexandraRoatis Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zhongduo Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

tayarani commented Jan 11, 2021

Uh oh!

AlexandraRoatis commented Jan 11, 2021

Uh oh!

tayarani commented Jan 11, 2021

Uh oh!

vaikas Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

vaikas commented Jan 12, 2021

Uh oh!

AlexandraRoatis commented Jan 12, 2021

Uh oh!

vaikas commented Jan 13, 2021

Uh oh!

knative-prow-robot commented Jan 13, 2021

Uh oh!

vaikas commented Jan 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AlexandraRoatis commented Nov 25, 2020 •

edited

Loading

aliok commented Nov 30, 2020 •

edited

Loading

codecov Bot commented Dec 21, 2020 •

edited

Loading