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

feat: reconcile eventtype v1beta3 resources #8087

Merged
merged 3 commits into from
Jul 11, 2024

Conversation

Cali0707
Copy link
Member

@Cali0707 Cali0707 commented Jul 9, 2024

Fixes #6754

This lets us reconcile eventtype v1beta3 resources with the updated eventtype v1beta3 logic.

Proposed Changes

  • If no reference is set, reconcile to ready
  • Reconcile on v1beta3 struct, not v1beta2
EventTypes no longer need a reference to be set on them

@knative-prow knative-prow bot requested review from creydr and pierDipi July 9, 2024 15:57
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 9, 2024
@Cali0707
Copy link
Member Author

Cali0707 commented Jul 9, 2024

/cc @matzew

One thing I was realizing while working on this is that maybe for the "required" id/specversion attributes we should default them, especially in the case of the "id" where we just want to say that it will be there but it doesn't make sense to give it a specific value

@knative-prow knative-prow bot requested a review from matzew July 9, 2024 15:58
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 67.82%. Comparing base (57b52ea) to head (574f73e).

Files Patch % Lines
pkg/reconciler/testing/v1/eventtype.go 80.00% 6 Missing and 3 partials ⚠️
pkg/apis/eventing/v1beta3/eventtype_lifecycle.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8087      +/-   ##
==========================================
+ Coverage   67.80%   67.82%   +0.02%     
==========================================
  Files         367      367              
  Lines       17373    17411      +38     
==========================================
+ Hits        11779    11809      +30     
- Misses       4859     4864       +5     
- Partials      735      738       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Calum Murray <[email protected]>
@matzew
Copy link
Member

matzew commented Jul 10, 2024

One thing I was realizing while working on this is that maybe for the "required" id/specversion attributes we should default them, especially in the case of the "id" where we just want to say that it will be there but it doesn't make sense to give it a specific value

type/source are also required.

I think we just need to make sure that they are around. Not sure on defaulting. One EventType describes a series of same/similar CloudEvents. It is not a 1-1 mapping. It is more a category, rather than a specific event

@matzew
Copy link
Member

matzew commented Jul 10, 2024

k get eventtypes.eventing.knative.dev
NAME                                  TYPE   SOURCE   REFERENCE NAME   REFERENCE KIND   DESCRIPTION   READY   REASON
dev.knative.source.github.push-v1b3                   default          Broker                         False   ResourceDoesNotExist

hrm...?

@Cali0707
Copy link
Member Author

k get eventtypes.eventing.knative.dev
NAME                                  TYPE   SOURCE   REFERENCE NAME   REFERENCE KIND   DESCRIPTION   READY   REASON
dev.knative.source.github.push-v1b3                   default          Broker                         False   ResourceDoesNotExist

hrm...?

@matzew I think the problem is that my branch didn't have the fix for the default reference on it, if you try again now it should work, I get:

k get eventtypes
NAME                                  TYPE   SOURCE   REFERENCE NAME   REFERENCE KIND   DESCRIPTION   READY   REASON
dev.knative.source.github.push-v1b3                                                                   True    

@Cali0707
Copy link
Member Author

/test reconciler-tests

@matzew
Copy link
Member

matzew commented Jul 11, 2024

also tried w/ the auto-create:

apiVersion: eventing.knative.dev/v1beta3
kind: EventType
metadata:
  annotations:
    eventing.knative.dev/creator: system:serviceaccount:knative-eventing:mt-broker-ingress
    eventing.knative.dev/lastModifier: system:serviceaccount:knative-eventing:mt-broker-ingress
  creationTimestamp: "2024-07-11T07:50:07Z"
  generation: 1
  name: et-default-3c4b2767ec442c946530776794ba99cb
  namespace: default
  ownerReferences:
  - apiVersion: eventing.knative.dev/v1
    kind: Broker
    name: default
    uid: 8abe927f-5876-4aac-a004-25d90c37a57f
  resourceVersion: "19273"
  uid: 5ca13b0b-cd5d-4597-85c5-04a06c1996c7
spec:
  attributes:
  - name: type
    required: true
    value: org.apache.matzew.superEvent
  - name: source
    required: true
    value: /some/source
  description: Event Type auto-created by controller
  reference:
    address: http
    apiVersion: eventing.knative.dev/v1
    kind: Broker
    name: default
    namespace: default
status:
  conditions:
  - lastTransitionTime: "2024-07-11T07:50:07Z"
    status: "True"
    type: Ready
  - lastTransitionTime: "2024-07-11T07:50:07Z"
    status: "True"
    type: ReferenceExists
  observedGeneration: 1

@matzew
Copy link
Member

matzew commented Jul 11, 2024

@Cali0707 for the CE id, specversion etc, we must add extra logic to capture this properly on the spec

Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2024
Copy link

knative-prow bot commented Jul 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, matzew

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 merged commit ea08684 into knative:main Jul 11, 2024
34 of 36 checks passed
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. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eventtype defaults to a "default" broker
2 participants