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

[380] enhance SonataFlowClusterPlatform.spec #389

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

tchughesiv
Copy link
Contributor

@tchughesiv tchughesiv commented Feb 8, 2024

Description of the change:
Enhance SonataFlowClusterPlatform.spec to better communicate which platform capabilities are being used by workflows cluster-wide. If spec.capabilities = nil, defaults to capabilities.workflows["services"].

apiVersion: sonataflow.org/v1alpha08
kind: SonataFlowClusterPlatform
metadata:
  name: cluster-platform
spec:
  platformRef:
    name: sonataflow-platform
    namespace: central-platform-ns
  capabilities:
    workflows:
    - services

Motivation for the change:
fixes #380

There are 2 goals in mind -

  1. Communicate, via the api, that workflows are the consumers of these cluster-wide platform capabilities. NOT services or any other future components.
  2. Communicate, via the api, WHICH capabilities are being made available cluster-wide. Currently, that would be services... but there could be more in the future, e.g. build configs.

Checklist

  • Add or Modify a unit test for your change
  • Have you verified that tall the tests are passing?
How to backport a pull request to a different branch?

In order to automatically create a backporting pull request please add one or more labels having the following format backport-<branch-name>, where <branch-name> is the name of the branch where the pull request must be backported to (e.g., backport-7.67.x to backport the original PR to the 7.67.x branch).

NOTE: backporting is an action aiming to move a change (usually a commit) from a branch (usually the main one) to another one, which is generally referring to a still maintained release branch. Keeping it simple: it is about to move a specific change or a set of them from one branch to another.

Once the original pull request is successfully merged, the automated action will create one backporting pull request per each label (with the previous format) that has been added.

If something goes wrong, the author will be notified and at this point a manual backporting is needed.

NOTE: this automated backporting is triggered whenever a pull request on main branch is labeled or closed, but both conditions must be satisfied to get the new PR created.

Copy link
Contributor

@wmedvede wmedvede left a comment

Choose a reason for hiding this comment

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

LGTM.

But, unless we are sure that other "capabilities" will managed at cluster level in the short term, and we have them more or less identified, I don't see why we need this.
I mean, right now, if you defile a SFCP is because you want to make DI and JS cluster wide, so, if you don't won't this capability, you don't define a SFCP, that's it.

@ricardozanini @tchughesiv would you mind guys elaborate which other capabilities we are going to define at cluster level and how they will work?

@ricardozanini
Copy link
Member

@wmedvede we can't preview the future atm, @wmedvede. But the core engine is already based on those capabilities in the form of Quarkus extensions. So, for instance, I can see cluster-level messaging, persistence, monitoring, and so on in the future. So leaving the API capable of accommodating those is the intention of this PR.

@jordigilh
Copy link
Contributor

@tchughesiv any plans to add E2E tests to this PR? So far there are none for this controller.

@tchughesiv
Copy link
Contributor Author

@tchughesiv any plans to add E2E tests to this PR? So far there are none for this controller.

@jordigilh @ricardozanini should i add them to this PR or a separate one?

@jordigilh
Copy link
Contributor

@tchughesiv any plans to add E2E tests to this PR? So far there are none for this controller.

@jordigilh @ricardozanini should i add them to this PR or a separate one?

Any reason for not adding them to this PR already? Perhaps dependencies to other unmerged PRs?

Copy link

@jakubschwan jakubschwan left a comment

Choose a reason for hiding this comment

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

lgtm

@tchughesiv
Copy link
Contributor Author

Any reason for not adding them to this PR already? Perhaps dependencies to other unmerged PRs?

@jordigilh let me see what i can do

@tchughesiv
Copy link
Contributor Author

@jordigilh @ricardozanini i've added e2e tests

@ricardozanini
Copy link
Member

@tchughesiv can you please rebase?

@tchughesiv
Copy link
Contributor Author

@ricardozanini done

@ricardozanini ricardozanini merged commit 64db02d into apache:main Feb 28, 2024
4 checks passed
rgdoliveira pushed a commit to rgdoliveira/kogito-serverless-operator that referenced this pull request Mar 11, 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
5 participants