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

fix(knative): Make knative broker name configurable #3373

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

claudio4j
Copy link
Contributor

@claudio4j claudio4j commented Jun 17, 2022

#2864

  • Add e2e knative tests
  • Change the kamelet-binding-broker yaks test to use a custom broker name

Release Note

Make knative broker name configurable

@claudio4j
Copy link
Contributor Author

@christophd @squakez for review.

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Nice, thanks! Let's keep this warm until the changes propagate to the runtime.

@squakez squakez added this to the 1.10.0 milestone Jun 20, 2022
@squakez squakez added the status/wip Work in progress label Jun 20, 2022
@claudio4j claudio4j force-pushed the fix_kameletbinding_brokername branch from 2b35825 to d4fa0f5 Compare June 20, 2022 15:36
Copy link
Member

@tadayosi tadayosi left a comment

Choose a reason for hiding this comment

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

Thanks!

@christophd
Copy link
Contributor

@claudio4j many thanks! some e2e tests failed. is this because the fixed Camel component is not part of Camel K yet?

@claudio4j
Copy link
Contributor Author

claudio4j commented Jun 21, 2022

some e2e tests failed. is this because the fixed Camel component is not part of Camel K yet?

Yes, we should wait the release train of camel artifacts.

@christophd
Copy link
Contributor

@claudio4j will users of the component be able to listen for all events on a broker with this fix, too? At the moment users need to filter for a given event type and can not listen for all events on that broker.

@claudio4j
Copy link
Contributor Author

claudio4j commented Jun 23, 2022

@claudio4j will users of the component be able to listen for all events on a broker with this fix, too?

Yes, there is a e2e knative-broker test that checks all messages are received.

At the moment users need to filter for a given event type and can not listen for all events on that broker.

No need for filtering to listen for all events, the following trigger spec was generated by camel-k-operator.

spec:
  broker: example-broker
  filter: {}
  subscriber:
    ref:
      apiVersion: serving.knative.dev/v1
      kind: Service
      name: receiver-example
      namespace: default
    uri: /events/

@christophd
Copy link
Contributor

@claudio4j awesome! this is a much appreciated enhancement

@tadayosi tadayosi mentioned this pull request Aug 22, 2022
@oscerd
Copy link
Contributor

oscerd commented Aug 23, 2022

@claudio4j can we rebase this please?

apache#2864

* Add e2e knative tests
* Change the kamelet-binding-broker yaks test to use a custom broker
  name
@claudio4j claudio4j force-pushed the fix_kameletbinding_brokername branch from d4fa0f5 to 2ff0172 Compare August 23, 2022 15:52
@claudio4j
Copy link
Contributor Author

@claudio4j can we rebase this please?

rebased.

@claudio4j claudio4j marked this pull request as ready for review August 23, 2022 15:54
@claudio4j
Copy link
Contributor Author

I see all of the failing CI tests are timeouts.

@squakez squakez removed the status/wip Work in progress label Aug 25, 2022
@squakez
Copy link
Contributor

squakez commented Aug 25, 2022

For what I could see, the check errors are the same we are experincing on main (likely releated to the new runtime?). I'm merging also this and we can focus on fixing directly any error on main

@squakez squakez merged commit e771b8a into apache:main Aug 25, 2022
@claudio4j claudio4j deleted the fix_kameletbinding_brokername branch August 29, 2022 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants