Workflow: Properties annotation #199

eloycoto · 2023-04-03T13:08:18Z

This commit introduces a new way to annotate workflow metadata to the workflow itself, the annotation comes with a single option, but with the idea to be expanded in the near future

Each workflow now has a way to define a version that is stored in the workflowDefinition.Properties column (JsonB) object. Users may want to determine the version by hand, or using any tool, like the example given using git-commit-id-plugin.

As an example:


@Bean(name = "simpleSequentialWorkFlow" + WorkFlowConstants.INFRASTRUCTURE_WORKFLOW)
@Infrastructure
@WorkFlowProperties(version = "${git.commit.id}")
WorkFlow simpleSequentialWorkFlowTask(@Qualifier("restCallTask") RestAPIWorkFlowTask restCallTask,
        @Qualifier("loggingTask") LoggingWorkFlowTask loggingTask) {

or:


@WorkFlowProperties(version = "v1.0.0")

The version is a string, but maybe we can think on semver here. I'm very open here, but the idea was to use the commit id AFAIK, and not sure if it adds value having in semver object.

The main reason to use Properties is that they can be extended with more fields like: @WorkFlowProperties(version = "v1.0.0", metricName="myAwesomeWorkflow") or some special logic like on_update, on_die, priority_key, labels.

Fix FLPATH-17

...ice/src/main/java/com/redhat/parodos/workflow/execution/aspect/WorkFlowPropertiesAspect.java

gabriel-farache

Very nicely done, left a few comments.

And would be good to have some piece of documentation somewhere explaining where to use and how it's working (like you've done in the description of the PR)

gabriel-farache · 2023-04-05T07:50:10Z

...ice/src/main/java/com/redhat/parodos/workflow/execution/aspect/WorkFlowPropertiesAspect.java

+		if (version.isEmpty()) {
+			version = WorkFlowVersionServiceImpl.GetVersionHashForObject(workFlow);
+		}


Why do you want to force a version on the workflow? That will be a bit confusing as a user as the default value is empty string on the annotation, I would expect to have a, empty version in the DB.
Maybe the default version should be the git commit sha?

If you want to keep this behaviour with a generated version based on the hash of the object, it would be great to have it documented somewhere

Yes, the hash is something that we agreed on in a previous meeting; it'll be documented in some way.

parodos-model-api/src/main/java/com/redhat/parodos/workflow/annotation/WorkFlowProperties.java

pkliczewski

minor comments, overall looks good

pkliczewski · 2023-04-05T07:53:38Z

workflow-examples/pom.xml

@@ -102,6 +107,32 @@
    </dependencies>
    <build>
        <plugins>
+            <plugin>
+                <groupId>pl.project13.maven</groupId>
+                <artifactId>git-commit-id-plugin</artifactId>


Can we use tag if available?

pkliczewski · 2023-04-05T07:54:40Z

...w-engine/src/main/java/com/redhat/parodos/workflows/workflow/WorkFlowPropertiesMetadata.java

+
+import lombok.Builder;
+
+@Builder


What about generating getter and setter?

What about generating getter and setter?
@DaTa has getter and setter

pkliczewski · 2023-04-05T07:56:26Z

workflow-service/src/main/java/com/redhat/parodos/project/controller/ProjectController.java

@@ -82,6 +82,7 @@ public ResponseEntity<ProjectResponseDTO> createProject(@Valid @RequestBody Proj
 					@ApiResponse(responseCode = "401", description = "Unauthorized", content = @Content),
 					@ApiResponse(responseCode = "403", description = "Forbidden", content = @Content) })
 	@GetMapping
+


is this extra line needed?

pkliczewski · 2023-04-05T07:58:19Z

...est/java/com/redhat/parodos/workflow/definition/dto/WorkFlowPropertiesDefinitionDTOTest.java

+import com.redhat.parodos.workflow.definition.entity.WorkFlowPropertiesDefinition;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;


What do you think about expanding the asterisk?

@bean

This commit introduces a new way to annotate workflow metadata to the workflow itself, the annotation comes with a single option, but with the idea to be expanded in the near future Each workflow now has a way to define a version that is stored in the workflowDefinition.Properties column (JsonB) object. Users may want to determine the version by hand, or using any tool, like the example given using git-commit-id-plugin. As an example: ``` @bean(name = "simpleSequentialWorkFlow" + WorkFlowConstants.INFRASTRUCTURE_WORKFLOW) @infrastructure @WorkFlowProperties(version = "${git.commit.id}") WorkFlow simpleSequentialWorkFlowTask(@qualifier("restCallTask") RestAPIWorkFlowTask restCallTask, @qualifier("loggingTask") LoggingWorkFlowTask loggingTask) { ``` or: ``` @WorkFlowProperties(version = "v1.0.0") ``` The version is a string, but maybe we can think on semver here. I'm very open here, but the idea was to use the commit id AFAIK, and not sure if it adds value having in semver object. The main reason to use Properties is that they can be extended with more fields like: `@WorkFlowProperties(version = "v1.0.0", metricName="myAwesomeWorkflow")` or some special logic like on_update, on_die, priority_key, labels. Fix FLPATH-17 Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>

Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>

pkliczewski

/lgtm

lshannon · 2023-04-05T11:50:22Z

/approve

pkliczewski · 2023-04-05T11:57:14Z

@eloycoto please rebase. Tide is complaining :)

RichardW98 · 2023-04-05T13:10:59Z

...rc/main/java/com/redhat/parodos/workflow/definition/entity/WorkFlowPropertiesDefinition.java

+public class WorkFlowPropertiesDefinition {
+
+	@Getter
+	@Setter


@DaTa has getter&setter, no need to add them here for each property again

RichardW98 · 2023-04-05T13:26:56Z

...ow-service/src/main/java/com/redhat/parodos/workflow/version/WorkFlowVersionServiceImpl.java

@@ -33,6 +33,11 @@
 @Slf4j
 public class WorkFlowVersionServiceImpl implements WorkFlowVersionService {

+	public static String GetVersionHashForObject(Object workflowObject) throws IOException {
+		WorkFlowVersionServiceImpl versionService = new WorkFlowVersionServiceImpl();


If you want to make it a Delegate/Util, try to make the method "getHash" static and remove "implements WorkFlowVersionService".

openshift-ci · 2023-04-05T14:37:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lshannon, RichardW98

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RichardW98,lshannon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress label Apr 3, 2023

eloycoto commented Apr 3, 2023

View reviewed changes

...ice/src/main/java/com/redhat/parodos/workflow/execution/aspect/WorkFlowPropertiesAspect.java Show resolved Hide resolved

eloycoto force-pushed the workFlowAspect branch from effab59 to d615ec1 Compare April 4, 2023 08:41

eloycoto requested review from RichardW98, gciavarrini and lshannon April 4, 2023 08:42

eloycoto marked this pull request as ready for review April 4, 2023 08:42

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 4, 2023

openshift-ci bot requested review from rgolangh and ydayagi April 4, 2023 08:42

pkliczewski added the needs-rebase label Apr 5, 2023

gabriel-farache reviewed Apr 5, 2023

View reviewed changes

pkliczewski reviewed Apr 5, 2023

View reviewed changes

eloycoto force-pushed the workFlowAspect branch from d615ec1 to e2b8853 Compare April 5, 2023 08:28

openshift-merge-robot removed the needs-rebase label Apr 5, 2023

eloycoto force-pushed the workFlowAspect branch from e2b8853 to 2f730ac Compare April 5, 2023 08:31

eloycoto added 3 commits April 5, 2023 12:06

Feat: add workflowProperties aspect tests

5a70ab9

Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>

fix: cleanup workflow-properties issues

c9f0d56

Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>

eloycoto force-pushed the workFlowAspect branch from 2f730ac to c9f0d56 Compare April 5, 2023 10:06

pkliczewski reviewed Apr 5, 2023

View reviewed changes

openshift-ci bot assigned pkliczewski Apr 5, 2023

openshift-ci bot added the lgtm label Apr 5, 2023

openshift-ci bot added the approved label Apr 5, 2023

pkliczewski added the needs-rebase label Apr 5, 2023

RichardW98 reviewed Apr 5, 2023

View reviewed changes

RichardW98 approved these changes Apr 5, 2023

View reviewed changes

openshift-merge-robot removed the needs-rebase label Apr 5, 2023

openshift-merge-robot merged commit 3f6667e into parodos-dev:main Apr 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow: Properties annotation #199

Workflow: Properties annotation #199

eloycoto commented Apr 3, 2023 •

edited by openshift-ci bot

Loading

gabriel-farache left a comment

gabriel-farache Apr 5, 2023

eloycoto Apr 5, 2023

pkliczewski left a comment

pkliczewski Apr 5, 2023

pkliczewski Apr 5, 2023

RichardW98 Apr 5, 2023

pkliczewski Apr 5, 2023

pkliczewski Apr 5, 2023

pkliczewski left a comment

lshannon commented Apr 5, 2023

pkliczewski commented Apr 5, 2023

RichardW98 Apr 5, 2023

RichardW98 Apr 5, 2023

openshift-ci bot commented Apr 5, 2023


		import lombok.Builder;

		@Builder

Workflow: Properties annotation #199

Workflow: Properties annotation #199

Conversation

eloycoto commented Apr 3, 2023 • edited by openshift-ci bot Loading

gabriel-farache left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkliczewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkliczewski left a comment

Choose a reason for hiding this comment

lshannon commented Apr 5, 2023

pkliczewski commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 5, 2023

eloycoto commented Apr 3, 2023 •

edited by openshift-ci bot

Loading