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

[issue-547] Knative Broker is required even if is not defined in SonataFlow or SonataFlowPlatform #553

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

jianrongzhang89
Copy link
Contributor

@jianrongzhang89 jianrongzhang89 commented Oct 16, 2024

This PR implements the following when Knative is installed:

  1. When there is no sink defined in a workflow, and there is no broker defined in the platform, the workflow reconciliation will continue instead of failing with errors.
  2. When a workflow does not have a broker defined in the workflow's spec.sources for the event type for events that the workflow consumes, and there is no broker configured in the platform, the workflow reconciliation will continue instead of failing with errors. In this case, no trigger will be created for that event type.

Motivation for the change:
This is for backward compatibility with the latest operator release and usability. If will be up to the user to decide whether a broker should be configured for a workflow.

Checklist

  • [ X] Add or Modify a unit test for your change
  • [ X] Have you verified that tall the tests are passing?

Fix #547

@jianrongzhang89 jianrongzhang89 changed the title [issue-547] Knative should not be required to install and run the SonataFlow Operator to deploy workflows [issue-547] Knative Broker is required even if is not defined in SonataFlow or SonataFlowPlatform Oct 16, 2024
Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

It seems pretty straightforward. Can you modify or add an e2e that validates it?

…d in the SonataFlow or SonataFlowPlatform CRs
@@ -230,7 +230,8 @@ var _ = Describe("Cluster Platform Use Cases :: ", Label("cluster"), Ordered, fu
Expect(err).NotTo(HaveOccurred())
},
Entry("without services configured", test.GetPathFromE2EDirectory("platform", "noservices"), metadata.GitOpsProfile.String(), ephemeral, false),
Entry("with services configured", test.GetPathFromE2EDirectory("platform", "services"), metadata.GitOpsProfile.String(), "ephemeral-with-workflow", true),
Entry("with services configured and platform broker", test.GetPathFromE2EDirectory("platform", "services"), metadata.GitOpsProfile.String(), "ephemeral-with-workflow", true),
Entry("with services configured and no broker", test.GetPathFromE2EDirectory("platform", "services"), metadata.GitOpsProfile.String(), "ephemeral-with-workflow-no-broker", true),
Copy link
Member

Choose a reason for hiding this comment

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

Since this test does not have a SonataFlowClusterPlatform, I think the e2e label might be incorrect.

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Please disregard my comment. The new test uses a clustered platform.

@ricardozanini
Copy link
Member

@wmedvede @jakubschwan can you please verify it?

@wmedvede wmedvede merged commit 957ca3d into apache:main Oct 31, 2024
4 checks passed
rgdoliveira pushed a commit to rgdoliveira/kogito-serverless-operator that referenced this pull request Nov 6, 2024
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.

Knative Broker is required even if is not defined in SonataFlow or SonataFlowPlatform
3 participants