You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2022/06/17 21:46:50 UTC
[GitHub] [camel-k] claudio4j opened a new pull request, #3373: fix(knative): Make knative broker name configurable
claudio4j opened a new pull request, #3373:
URL: https://github.com/apache/camel-k/pull/3373
https://github.com/apache/camel-k/issues/2864
* Add e2e knative tests
* Change the kamelet-binding-broker yaks test to use a custom broker name
**Release Note**
```release-note
Make knative broker name configurable
```
* WIP: This PR requires a fix in camel-knative (to be submitted). This is for an early review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] oscerd commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
oscerd commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1223528265
@claudio4j can we rebase this please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on a diff in pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
claudio4j commented on code in PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#discussion_r901802474
##########
e2e/knative/knative_test.go:
##########
@@ -118,6 +118,20 @@ func TestRunBroker(t *testing.T) {
})
}
+func TestRunCustomBroker(t *testing.T) {
Review Comment:
Done, thanks for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
squakez commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1226942015
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
christophd commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1164462107
@claudio4j awesome! this is a much appreciated enhancement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez merged pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
squakez merged PR #3373:
URL: https://github.com/apache/camel-k/pull/3373
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1161846661
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1225775067
I see all of the failing CI tests are timeouts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1224268133
> @claudio4j can we rebase this please?
rebased.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
christophd commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1161840034
@claudio4j many thanks! some e2e tests failed. is this because the fixed Camel component is not part of Camel K yet?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1159523638
@christophd @squakez for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on a diff in pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
tadayosi commented on code in PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#discussion_r901370294
##########
e2e/knative/knative_test.go:
##########
@@ -118,6 +118,20 @@ func TestRunBroker(t *testing.T) {
})
}
+func TestRunCustomBroker(t *testing.T) {
Review Comment:
For E2E we prefer `t.Run(...)` over multiple `func Test...` test functions. Isn't it something we can merge into a single `TestRunBroker(...)`?
Also it'd be great if we could take this opportunity to combine the existing test functions into `TestRunBroker(...)` as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1164413704
> @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.
Yes, there is a [e2e knative-broker](https://github.com/apache/camel-k/blob/main/e2e/yaks/common/knative-broker/knative-broker.feature#L45-L47) test that checks all messages are received.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #3373: fix(knative): Make knative broker name configurable
Posted by GitBox <gi...@apache.org>.
christophd commented on PR #3373:
URL: https://github.com/apache/camel-k/pull/3373#issuecomment-1164141812
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org