eventing icon indicating copy to clipboard operation
eventing copied to clipboard

Enable serving of EventTypes v1beta3

Open dsimansk opened this issue 2 years ago • 7 comments

To enable further work on EventType changes, ref https://github.com/knative/eventing/pull/7423.

/cc @cardil @matzew

Proposed Changes

  • Enable serving of EventTypes v1beta3

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


Docs

dsimansk avatar Jan 16 '24 16:01 dsimansk

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk

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:

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

knative-prow[bot] avatar Jan 16 '24 16:01 knative-prow[bot]

@dsimansk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
unit-tests_eventing_main 23fecdcc672fbd030854a2d2c8d51074a91f15ab link true /test unit-tests
reconciler-tests_eventing_main 23fecdcc672fbd030854a2d2c8d51074a91f15ab link true /test reconciler-tests

Your PR dashboard.

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. I understand the commands that are listed here.

knative-prow[bot] avatar Jan 16 '24 17:01 knative-prow[bot]

AFAIK, the new version has some API changes on the types and I don't see them reflected here yet https://github.com/knative/eventing/blob/main/pkg/apis/eventing/v1beta3/eventtype_types.go, I guess we need to start with that first ?

@pierDipi I believe that currently the v1beta3 types haven't changed, the only thing that will change will be the default behaviour and the reconciling without a reference

Cali0707 avatar Jan 16 '24 19:01 Cali0707

We cannot change that reconciling without a reference behavior on a single version as we wouldn't know which version the user has created

pierDipi avatar Jan 17 '24 07:01 pierDipi

AFAIK, the new version has some API changes on the types and I don't see them reflected here yet https://github.com/knative/eventing/blob/main/pkg/apis/eventing/v1beta3/eventtype_types.go, I guess we need to start with that first ?

@pierDipi I believe that currently the v1beta3 types haven't changed, the only thing that will change will be the default behaviour and the reconciling without a reference

@pierDipi Reckoning a bit about the changes, we at least can drop deprecated Broker field in v1b3, right? Anything else missing?

dsimansk avatar Jan 17 '24 10:01 dsimansk

We could but we need to make sure we have a strategy to have a lossless conversion between versions.

Another thought, if we have an already improved API design why not going with that and avoid introducing another version just to drop a deprecated field ?

pierDipi avatar Jan 17 '24 13:01 pierDipi

We could but we need to make sure we have a strategy to have a lossless conversion between versions.

Another thought, if we have an already improved API design why not going with that and avoid introducing another version just to drop a deprecated field ?

The idea was to fix defaulting behavior in v1b3 landing in upcoming release v1.13. Then in the next cycle on v1b4 refactor the spec field to include more Cloud Events fields in a key-value map style. Hence no to rush the spec refactor as it's significantly bigger and riskier change. |

Would you rather do it in one swing, i.e. all changes in v1b3 and included in release v1.14 only?

dsimansk avatar Jan 17 '24 14:01 dsimansk

This Pull Request is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen with /reopen. Mark as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Apr 17 '24 01:04 github-actions[bot]

That's superseded by other more recent PRs.

/close

dsimansk avatar Apr 17 '24 09:04 dsimansk

@dsimansk: Closed this PR.

In response to this:

That's superseded by other more recent PRs.

/close

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.

knative-prow[bot] avatar Apr 17 '24 09:04 knative-prow[bot]