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

Add section to configure when operator full knative #207

Closed

Conversation

gabriel-farache
Copy link
Contributor

Closes https://issues.redhat.com/browse/FLPATH-1406
Add section to configure when operator full knative

Related to parodos-dev/serverless-workflows-config#375

@gabriel-farache
Copy link
Contributor Author

/hold until apache/incubator-kie-kogito-serverless-operator#467 is merged and chart is updated with operator version with the feature

Copy link
Contributor

@masayag masayag left a comment

Choose a reason for hiding this comment

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

We agreed not incorporate the Kafka related configuration into the chart, but perhaps patching the sonataflowplatform can be considered, WDYT?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

Kafka perfectly fullfil this reliability need.

For that, you will need:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the entire orchestrator chart is designed for OCP (there is another one for k8s), I'd try to remain as closer as possible to the formal OCP admin guides.
Are the following steps fits with the recommendation in the OCP admin guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WDYM? You do not like the wording For that, you will need: ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that better?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring only to the steps to configure Kafka as a broker of knative eventing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, ok
So, is that better now?

Copy link
Contributor

Choose a reason for hiding this comment

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

My intention was to provide a link to https://docs.openshift.com/serverless/1.33/eventing/brokers/kafka-broker.html which is the official admin guide for openshift serverless instead of relying on the upstream doc.
Mainly since we are opinionated about the version of the openshift serverless operators installed by the chart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the link to this doc in step 2 when we create the broker

But I think we still need to install the eventing resources we install at step 1, not sure if everything is installed with the operator

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@gabriel-farache
Copy link
Contributor Author

gabriel-farache commented Jul 1, 2024

@masayag

We agreed not incorporate the Kafka related configuration into the chart, but perhaps patching the sonataflowplatform can be considered

What is related to kafka that should be in the sonataflowplatform? In the readme, we are already patching it for the broker: https://github.com/parodos-dev/orchestrator-helm-chart/pull/207/files#diff-75c29d9ec0c1a79cafdacdbbebff3ea3fa3d649f1b43440783208b21845238dcR61-R76

@masayag
Copy link
Contributor

masayag commented Jul 1, 2024

@masayag

We agreed not incorporate the Kafka related configuration into the chart, but perhaps patching the sonataflowplatform can be considered

What is related to kafka that should be in the sonataflowplatform? In the readme, we are already patching it for the broker: https://github.com/parodos-dev/orchestrator-helm-chart/pull/207/files#diff-75c29d9ec0c1a79cafdacdbbebff3ea3fa3d649f1b43440783208b21845238dcR61-R76

I was trying to explore the option to incorporate this patch into the orchestrator chart, but we can start without having it included.

@gabriel-farache
Copy link
Contributor Author

was trying to explore the option to incorporate this patch into the orchestrator chart, but we can start without having it included.

The issue here is that the broker name depends on the user
We could also include all the steps to the orchestrator directly (installing the knative resource for kafka, creating and configuring the kafka broker and patchin the sonataflowplateform) if the user asks for it

But that should be another issue

Signed-off-by: gabriel-farache <[email protected]>
Signed-off-by: gabriel-farache <[email protected]>
Signed-off-by: gabriel-farache <[email protected]>
Signed-off-by: gabriel-farache <[email protected]>
@masayag
Copy link
Contributor

masayag commented Sep 9, 2024

pls reopen this PR in https://github.com/parodos-dev/orchestrator-helm-operator

Signed-off-by: gabriel-farache <[email protected]>
Signed-off-by: gabriel-farache <[email protected]>
@masayag
Copy link
Contributor

masayag commented Sep 11, 2024

@gabriel-farache this PR needs to be closed and reopened against https://github.com/parodos-dev/orchestrator-helm-operator

@gabriel-farache
Copy link
Contributor Author

@masayag I know I know, it's on my TODO list, but it's not top priority as the PR is still in progress

@gabriel-farache
Copy link
Contributor Author

@masayag , there I did it, now we have 0 PR on this repo :)

@masayag
Copy link
Contributor

masayag commented Sep 12, 2024

@masayag , there I did it, now we have 0 PR on this repo :)

thanks @gabriel-farache, now there are also 0 issues ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants