Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Review eventing doc and added automated installation script #498

Merged

Conversation

gabriel-farache
Copy link
Collaborator

Review eventing doc and added automated installation script

## Automated installation steps
Usage:
```
Usage: ORCHESTRATOR_NAME=ORCHESTRATOR_NAME BROKER_NAME=BROKER_NAME BROKER_NAMESPACE=BROKER_NAMESPACE [KAFKA_REPLICATION_FACTOR=KAFKA_REPLICATION_FACTOR] [ORCHESTRATOR_NAMESPACE=openshift-operators] [BROKER_TYPE=kafka] [INSTALL_KAFKA_CLUSTER=true] ./eventing-automate-install.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you breakdown this line into multiple lines? it is rendered as a very long line.
In addition, the copy button will copy everything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's the usage text, it does not aim to be copy/paste, instead the commands below should be used

ORCHESTRATOR_NAMESPACE Optional, namespace in which the orchestrator operator is depoyed. Default is openshift-operators
BROKER_NAME Name of the broker to install
BROKER_NAMESPACE Namespace in which the broker must be installed
BROKER_TYPE Optional , type of the broker. Either 'kafka' or 'in-memory'. Default is: 'kafka'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the broker type be other than in-memory or kafka?
Perhaps this can be named IN_MEMORY_BROKER with a boolean value, with false as a default?
Being explicit is also fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can the broker type be other than in-memory or kafka

Yes https://knative.dev/docs/eventing/brokers/broker-types/
But I think we do not want to support it

I like being explicit so if we have to support a new type of broker it will be easier and limit the amount of env var to set

Copy link
Collaborator

@jenniferubah jenniferubah left a comment

Choose a reason for hiding this comment

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

Looks fine

Copy link
Collaborator

@ElaiShalevRH ElaiShalevRH left a comment

Choose a reason for hiding this comment

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

One last comment but otherwise lgtm

Signed-off-by: gabriel-farache <gfarache@redhat.com>
@gabriel-farache gabriel-farache merged commit 239586e into rhdhorchestrator:main Jan 29, 2025
@gabriel-farache gabriel-farache deleted the feat/automate_eventing branch January 29, 2025 08:55
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants