Skip to content

feat: introduce @Workflow annotation #2209

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

Closed
wants to merge 11 commits into from
Closed

feat: introduce @Workflow annotation #2209

wants to merge 11 commits into from

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Jan 18, 2024

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 18, 2024
@csviri csviri self-assigned this Jan 18, 2024
@@ -203,7 +203,7 @@ private static List<DependentResourceSpec> dependentResources(
ControllerConfiguration<?> parent) {
final var dependents =
valueOrDefault(annotation,
io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration::dependents,
c -> c.workflow().dependents(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will crash if there is no workflow annotation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a default workflow annotation, with empty dependents. To my understanding that never happens.

import io.javaoperatorsdk.operator.processing.event.source.EventSource;
import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource;

import java.util.Map;
import java.util.Optional;

@ControllerConfiguration(dependents = {@Dependent(type = ConfigMapDependentResource.class)})
@ControllerConfiguration(
workflow = @Workflow(dependents = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

TBH, I'm not convinced that this improves anything. This is more verbose and will require quite a bit of change in lots of places for non-obvious benefits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is required also for this issue:
#1898

That is why it is not just a simple rename.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't follow how this is required for explicit invocation of workflows. There's an implicit workflow being created already so adding an annotation doesn't bring much unless we would like to have data specifically associated with the workflow. That said, even workflow data could be added to the ControllerConfiguration annotation. Adding a new annotation would only make sense if we envisioned to have lots of data associated with workflows and/or if we wanted to have several workflows with different data set, which I don't think we should in either cases.

Copy link
Collaborator Author

@csviri csviri Feb 13, 2024

Choose a reason for hiding this comment

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

There are multiple aspects of this, why I think this is really beneficial:

  1. what is probably less important is just better domain language, when talking to devs usually they don't get that in the background there is this Workflow that is executes the Dependent resources, so would be nice to have both in the domain language.

  2. Features like this:

@ControllerConfiguration(
    workflow = @Workflow( 
          explicitInvocation = true,
          dependents = {
              @Dependent(...),
              @Dependent(...)}
    )

In the mentioned link, this is closely related to the workflow, and how it is invoked. It is much nicer to have such configs / feature options put there since its is coupled with the workflow not the controller configuration directly.
I expect that more of such will come in the future, so it is kinda future compatibility also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't follow how this is required for explicit invocation of workflows.

So if there is no flag like mentioned how can the controller know if the workflow should be executed before reconcile method is invoked or it will be invoked manually / explicitly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't follow how this is required for explicit invocation of workflows.

So if there is no flag like mentioned how can the controller know if the workflow should be executed before reconcile method is invoked or it will be invoked manually / explicitly.

By simply having an explicitWorkflowInvocation attribute on the ControllerConfiguration annotation without the need for an intermediate annotation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are multiple aspects of this, why I think this is really beneficial:

1. what is probably less important is just better domain language, when talking to devs usually they don't get that in the background there is this Workflow that is executes the Dependent resources, so would be nice to have both in the domain language.

Sure but this could be addressed by better documentation 😄

2. Features like [this](https://github.com/operator-framework/java-operator-sdk/issues/1898):
@ControllerConfiguration(
    workflow = @Workflow( 
          explicitInvocation = true,
          dependents = {
              @Dependent(...),
              @Dependent(...)}
    )

In the mentioned link, this is closely related to the workflow, and how it is invoked. It is much nicer to have such configs / feature options put there since its is coupled with the workflow not the controller configuration directly. I expect that more of such will come in the future, so it is kinda future compatibility also.

What other attributes would we be adding to the workflow configuration? I don't think that we should be adding complexity just for the sake of potential future features which might never materialize.

Copy link
Collaborator Author

@csviri csviri Feb 13, 2024

Choose a reason for hiding this comment

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

see also: #2238 (comment)

It is much nicer and descriptive to have these coupled.

Not sure what is wrong with an intermediate annotation. It is much nicer domain modeling, if we have hierachical structure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure but this could be addressed by better documentation 😄

I think we all agree if it is much better if the user does not have to dive into docs, just understands the code based how it is structured.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What other attributes would we be adding to the workflow configuration? I don't think that we should be adding complexity just for the sake of potential future features which might never materialize.

So the explicit domain objects reduce cognitive burden imo, as mentioned, users will create a @Workflow and will see what are the related properties and config options. So they don't have to search for (and potintially miss it) in @ControllerConfiguratio

@csviri csviri force-pushed the workflow-annotation branch 2 times, most recently from 77db332 to 0bf5e21 Compare January 23, 2024 14:23
@csviri csviri requested a review from metacosm January 23, 2024 14:24
@csviri csviri force-pushed the workflow-annotation branch from 0bf5e21 to af0c3c2 Compare January 25, 2024 10:35
@csviri csviri marked this pull request as ready for review January 25, 2024 10:41
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 25, 2024
csviri and others added 5 commits February 6, 2024 10:43
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
@csviri csviri force-pushed the workflow-annotation branch from 65deaf2 to 0bc24ec Compare February 13, 2024 09:08
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2024
@openshift-merge-robot
Copy link

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@csviri
Copy link
Collaborator Author

csviri commented Mar 8, 2024

replaced by: #2274

@csviri csviri closed this Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Invocation of Managed Workflows Rename dependents to workflow in @ControllerConfiguration in v5
3 participants