Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable serving of EventTypes v1beta3 #7583

Closed
wants to merge 1 commit into from

Conversation

dsimansk
Copy link
Contributor

To enable further work on EventType changes, ref #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

@knative-prow knative-prow bot requested review from cardil and matzew January 16, 2024 16:26
Copy link

knative-prow bot commented Jan 16, 2024

[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 knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 16, 2024
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link

knative-prow bot commented Jan 16, 2024

@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 23fecdc link true /test unit-tests
reconciler-tests_eventing_main 23fecdc 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.

@Cali0707
Copy link
Member

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
Copy link
Member

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

@dsimansk
Copy link
Contributor Author

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?

@pierDipi
Copy link
Member

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 ?

@dsimansk
Copy link
Contributor Author

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?

Copy link

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 github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 17, 2024
@dsimansk
Copy link
Contributor Author

That's superseded by other more recent PRs.

/close

Copy link

knative-prow bot commented Apr 17, 2024

@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 knative-prow bot closed this Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants