You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "claudio4j (via GitHub)" <gi...@apache.org> on 2023/03/10 18:20:12 UTC
[GitHub] [camel-k] claudio4j opened a new pull request, #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
claudio4j opened a new pull request, #4120:
URL: https://github.com/apache/camel-k/pull/4120
https://github.com/apache/camel-k/issues/4119
**Release Note**
```release-note
knative trait sets `bindings.knative.dev/include=true` label to the namespace when there is a SinkBinding.
```
--
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 #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
claudio4j commented on code in PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#discussion_r1133923001
##########
config/rbac/operator-role.yaml:
##########
@@ -71,6 +71,7 @@ rules:
- configmaps
- secrets
- serviceaccounts
+ - namespaces
Review Comment:
Ops, fixed that. Thanks for spotting it.
--
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 #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
claudio4j commented on code in PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#discussion_r1133944210
##########
config/rbac/operator-role.yaml:
##########
@@ -80,6 +80,13 @@ rules:
- patch
- update
- watch
+- apiGroups:
Review Comment:
I thought about it, but having the `apiGroup: ""` not in any of the knative objects, then maybe this role could be set in the camel-k-operator, but having it as you suggest would be fine too.
--
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 #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
claudio4j commented on code in PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#discussion_r1133823708
##########
e2e/yaks/common/knative-sinkbinding-http/sinkbinding.feature:
##########
@@ -0,0 +1,9 @@
+Feature: Camel K can run source HTTP endpoint in sinkbinding mode
+
+ Background:
+ Given Kubernetes resource polling configuration
+ | maxAttempts | 1 |
+ | delayBetweenAttempts | 500 |
+
+ Scenario: Integration knative-service starts with no errors
+ Given wait for condition=Ready on Kubernetes custom resource integration/rest2channel in integration.camel.apache.org/v1
Review Comment:
To provoke the error, the integration starts and keeps on `Running` phase for some seconds to later fail with `java.lang.IllegalArgumentException: Unable to find a resource definition for channel/sink/messages`, then the integration ping pong between Running and Error. In this case the yaks test `Given Camel K integration` returns true when it matches the `Running` phase.
I found out using the custom k8s resource test worked best.
--
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] astefanutti commented on a diff in pull request #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "astefanutti (via GitHub)" <gi...@apache.org>.
astefanutti commented on code in PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#discussion_r1133929082
##########
config/rbac/operator-role.yaml:
##########
@@ -80,6 +80,13 @@ rules:
- patch
- update
- watch
+- apiGroups:
Review Comment:
What about adding it in the `camel-k-operator-knative` role, as it's strictly needed for Knative, to keep the RBAC modularity consistent?
--
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 a diff in pull request #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#discussion_r1133575030
##########
e2e/yaks/common/knative-sinkbinding-http/sinkbinding.feature:
##########
@@ -0,0 +1,9 @@
+Feature: Camel K can run source HTTP endpoint in sinkbinding mode
+
+ Background:
+ Given Kubernetes resource polling configuration
+ | maxAttempts | 1 |
+ | delayBetweenAttempts | 500 |
+
+ Scenario: Integration knative-service starts with no errors
+ Given wait for condition=Ready on Kubernetes custom resource integration/rest2channel in integration.camel.apache.org/v1
Review Comment:
Why not use the Camel K step `Given Camel K integration rest2channel is running`?
--
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] astefanutti commented on a diff in pull request #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "astefanutti (via GitHub)" <gi...@apache.org>.
astefanutti commented on code in PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#discussion_r1133590182
##########
config/rbac/operator-role.yaml:
##########
@@ -71,6 +71,7 @@ rules:
- configmaps
- secrets
- serviceaccounts
+ - namespaces
Review Comment:
It doesn't seem like a best practice to grant all verbs on `namespaces` resources, especially `delete` and `deletecollection`. Could this be refined to the minimal set of verbs?
--
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 #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
claudio4j commented on code in PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#discussion_r1135532437
##########
config/rbac/operator-role.yaml:
##########
@@ -80,6 +80,13 @@ rules:
- patch
- update
- watch
+- apiGroups:
Review Comment:
For me it's fine having the permission defined in operator-knative.yaml, in the end it's related to the knative interoperability, then it's fine to be there.
--
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 #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
claudio4j commented on PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#issuecomment-1468093605
Can we merge it ?
--
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] astefanutti commented on a diff in pull request #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "astefanutti (via GitHub)" <gi...@apache.org>.
astefanutti commented on code in PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#discussion_r1133947864
##########
config/rbac/operator-role.yaml:
##########
@@ -80,6 +80,13 @@ rules:
- patch
- update
- watch
+- apiGroups:
Review Comment:
I see what you mean. Then having a new role with an explicit name, so it's clear it's not part of the core / mandatory roles, would work even better IMO.
--
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] astefanutti commented on a diff in pull request #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "astefanutti (via GitHub)" <gi...@apache.org>.
astefanutti commented on code in PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#discussion_r1135632114
##########
config/rbac/operator-role.yaml:
##########
@@ -80,6 +80,13 @@ rules:
- patch
- update
- watch
+- apiGroups:
Review Comment:
Sounds good to me too.
--
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 merged pull request #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
claudio4j merged PR #4120:
URL: https://github.com/apache/camel-k/pull/4120
--
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 #4120: fix(knative): Integration in error state when knative SinkBinding mode=inclusion
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
claudio4j commented on PR #4120:
URL: https://github.com/apache/camel-k/pull/4120#issuecomment-1466018189
> I remember that we have already fixed something regarding namespace labelling for Knative sink binding injection in the past. Do you know why this came back as a regression?
It was fixed only on 1.8 downstream at the time.
--
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