Add a workflow task that sends a message to topic in TIBCO #123

ydayagi · 2023-03-17T15:53:45Z

FLPATH-145 https://issues.redhat.com/browse/FLPATH-145

workflow-examples/README.md

gciavarrini · 2023-03-20T08:03:10Z

tasks/src/main/java/com/redhat/parodos/tasks/TibcoWorkFlowTask.java

+	private Tibjms service;
+	private String url;
+	private String caFile;
+	private String username;
+	private String password;


I'd suggest to make these private final

@gciavarrini fixed

gciavarrini · 2023-03-20T08:04:02Z

tasks/src/test/java/com/redhat/parodos/tasks/TibcoWorkFlowTaskTest.java

+	String caFile = "/cafile";
+	String url = "ssl://localhost:7222";
+	String username = "username";
+	String passowrd = "password";
+	String topic = "topic";
+	String message = "test message";


I'd suggest to make this private final

private = it's a best practice to keep the scope as narrow as you can.
final = It's a good practice to declare final a variable to be sure that it won't be modified unintentionally, and in addition it makes the understanding of code easier.

TBH this strings can be declared private static final so we're stating that such variables are constants and that they are associated with the class (not the instance of the class)

@gciavarrini fixed

gciavarrini · 2023-03-20T08:09:20Z

tasks/src/test/java/com/redhat/parodos/tasks/TibcoWorkFlowTaskTest.java

+	Tibjms tibjms = Mockito.mock(Tibjms.class);
+	TibcoWorkFlowTask task = new TibcoWorkFlowTask(tibjms, url, caFile, username, passowrd);


It's a good practice to initialize objects in @Before method.
In this way, the object is recreated before each test.

In addition, make these objects private as well.

good practice? I don't think so. Not in this case. In the case of simple initialization, we have the Java language that works for us. You don't need to use additional libraries.
Why make them private? No one is going to use this class outside the test. The default is package-private.

If you modify the object inside one of tests, then following tests will use a "dirty" object.
Regarding the private = It's a good practice to start to use the narrow privilege and the increase it if needed (so, private is more restrictive then package-private). Ideally, a good practice is to create immutable classes, but I understand that we can be flexible in this.

The Before does not solve it. It only happens once in the beginning. If test A changes something then test B will see a dirty obj.

I think you're confusing @Before with @BeforeClass

public class Example { @BeforeClass public static void classSetUp(){ System.out.println("Before Class"); } @Before public void setUp(){ System.out.println("Before"); } @Test public void test1(){ System.out.println("Test 1"); } @Test public void test2(){ System.out.println("Test 2"); } }

when you execute all the tests in the Example class then they print:

Before Class Before Test 1 Before Test 2

tasks/src/test/java/com/redhat/parodos/tasks/TibcoWorkFlowTaskTest.java

gciavarrini · 2023-03-20T08:18:41Z

workflow-examples/src/main/java/com/redhat/parodos/examples/TibcoWorkFlowConfiguration.java

@@ -0,0 +1,34 @@
+// @formatter:off


why is it necessary to disable the formatter for the whole file?

so I don't need to execute it for every change. I don't need this formatter.

that will eventually end as s project with multiple formatting while the convention is to use spring formatted.
while I find it PITA to keep running the formatted, we'd better handle auto-format rather than skip it.

why not skip?

@gciavarrini fixed

It is still there. Can you double check?

nirarg · 2023-03-20T09:18:48Z

pom.xml

@@ -62,6 +62,7 @@
        <module>notification-service</module>
        <module>pattern-detection-library</module>
        <module>coverage</module>
+        <module>tasks</module>


This module is going to be used for all common tasks
I think it should be renamed to more informative name
I suggest: workflow-common-tasks

We know it is common. We know it is for workflow. Why add all this text? I can somehow understand the addition of 'workflow-'. Some how ...

We know its common, I'm not sure how clear it would be for new users, I think its better to make it explicit so it would more clear.
Anyway, I don't have any problem with changing to workflow-tasks

used prebuilt-tasks. workflow-tasks collides with existing workflow task objects

@nirarg fixed

nirarg · 2023-03-20T09:25:41Z

tasks/src/main/java/com/redhat/parodos/tasks/Tibjms.java

I suggest to keep this module directory only with the tasks files
Remove all other utils files to another sub-directory

it only makes it more complex. I will move the whole tibco task to a sub-directory

I think, for the long term, we would like to avoid having one directory contains many files
Need to keep it clean, maybe should use here more hierarchical folders

more hierarchy more problems more difficulties reading and following files

ydayagi · 2023-03-20T09:51:31Z

/retest

masayag · 2023-03-20T13:16:08Z

external-dependencies/download-tibco.sh

@@ -0,0 +1,10 @@
+#!/bin/bash
+if [ -f "tibjms.jar" ]; then exit 0; fi
+rm -rf /tmp/tibco


nit: maybe mktemp -d tibco-XXX ?
(just in case there are few build processes running on the same machine)

really? isn't each build in its own container?

I don't know. maybe @eloycoto knows?

pom.xml

prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/tibco/TibjmsImpl.java

masayag · 2023-03-20T13:24:30Z

prebuilt-tasks/pom.xml

+            <version>1.18.24</version>
+        </dependency>
+        <dependency>
+            <groupId>org.springframework</groupId>


I haven't seen a direct use of this dependency.

I did not add it. I think it is for the '@slf4j'

'@slf4j' comes from lombok. pls try to build without that dependency and see if it is actually being used.

prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/tibco/Tibjms.java

prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/tibco/TibjmsImpl.java

prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/tibco/TibcoWorkFlowTaskTest.java

workflow-examples/README.md

masayag · 2023-03-20T13:33:42Z

workflow-examples/pom.xml

+            <groupId>dev.parodos</groupId>
+            <artifactId>prebuilt-tasks</artifactId>
+            <version>${revision}</version>
+            <scope>compile</scope>


compile is the default scope. It can be removed.

@masayag fixed

masayag · 2023-03-20T13:35:54Z

workflow-examples/src/main/java/com/redhat/parodos/examples/TibcoWorkFlowConfiguration.java

@@ -0,0 +1,34 @@
+// @formatter:off


that will eventually end as s project with multiple formatting while the convention is to use spring formatted.
while I find it PITA to keep running the formatted, we'd better handle auto-format rather than skip it.

masayag · 2023-03-20T19:51:50Z

prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/tibco/TibcoWorkFlowTask.java

+			String topic = getParameterValue(workContext, "topic");
+			String message = getParameterValue(workContext, "message");
+
+			log.debug("sending to topic " + topic + " message: " + message);


instead of string concatenation, use

log.debug("sending to topic {} message: {}", topic, message);

and that's why?

that's what preventing approval of this PR?

prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/tibco/TibcoWorkFlowTaskTest.java

gciavarrini · 2023-03-21T08:25:18Z

prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/tibco/TibcoWorkFlowTaskTest.java

+			map = Map.of("topic",topic,"message",message);
+		} else {
+			map = new HashMap();
+		}


Why do not create the HashMap before the if and then add value if withParams is true ?

HashMap<String, String> map = new HashMap<>(); if(withParams){ map.put("topic", topic); map.put("message", message); }

In addition, map is a variable name too generic, what about messageMap or something more "declarative"?

why? see my previous comment. I changed it just so we can continue

prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/tibco/TibcoWorkFlowTask.java

gciavarrini · 2023-03-21T08:31:46Z

workflow-examples/src/main/java/com/redhat/parodos/examples/TibcoWorkFlowConfiguration.java

@@ -0,0 +1,34 @@
+// @formatter:off


It is still there. Can you double check?

prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/tibco/TibcoWorkFlowTaskTest.java

gciavarrini · 2023-03-21T08:33:04Z

prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/tibco/TibcoWorkFlowTaskTest.java

+	private final String caFile = "/cafile";
+	private final String url = "ssl://localhost:7222";
+	private final String username = "username";
+	private final String passowrd = "password";
+	private final String topic = "topic";
+	private final String message = "test message";


missing static as per our previous discussion :)

does it really matter?

masayag · 2023-03-22T13:13:41Z

lgtm

FLPATH-145 https://issues.redhat.com/browse/FLPATH-145 Signed-off-by: Yaron Dayagi <ydayagi@redhat.com>

gciavarrini · 2023-03-22T16:25:33Z

/lgtm

pkliczewski · 2023-03-23T08:05:26Z

@gciavarrini please remove requested changes

pkliczewski · 2023-03-23T08:06:18Z

/approve

gciavarrini

/lgtm

openshift-ci · 2023-03-23T08:07:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gciavarrini, pkliczewski, ydayagi

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~~ [pkliczewski]

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 requested review from gciavarrini and nirarg March 17, 2023 15:53

masayag reviewed Mar 20, 2023

View reviewed changes

workflow-examples/README.md Outdated Show resolved Hide resolved

ydayagi force-pushed the flpath145 branch 2 times, most recently from 2b29fe1 to 7a96bf6 Compare March 20, 2023 08:16

gciavarrini suggested changes Mar 20, 2023

View reviewed changes

openshift-ci bot assigned gciavarrini Mar 20, 2023

ydayagi force-pushed the flpath145 branch from 7a96bf6 to 40e2aff Compare March 20, 2023 08:28

nirarg reviewed Mar 20, 2023

View reviewed changes

ydayagi force-pushed the flpath145 branch 3 times, most recently from b8e2d18 to 1b60873 Compare March 20, 2023 12:14

masayag reviewed Mar 20, 2023

View reviewed changes

ydayagi force-pushed the flpath145 branch from 1b60873 to 59ad51d Compare March 20, 2023 14:34

masayag reviewed Mar 20, 2023

View reviewed changes

gciavarrini reviewed Mar 21, 2023

View reviewed changes

ydayagi force-pushed the flpath145 branch 5 times, most recently from 709c466 to a527101 Compare March 22, 2023 10:33

ydayagi force-pushed the flpath145 branch from a527101 to 14dc228 Compare March 22, 2023 13:21

rgolangh mentioned this pull request Mar 22, 2023

Shell runner tasks #113

Merged

Add a workflow task that sends a message to topic in TIBCO

0e0484a

FLPATH-145 https://issues.redhat.com/browse/FLPATH-145 Signed-off-by: Yaron Dayagi <ydayagi@redhat.com>

ydayagi force-pushed the flpath145 branch from 14dc228 to 0e0484a Compare March 22, 2023 15:36

openshift-ci bot added the lgtm label Mar 22, 2023

openshift-ci bot added the approved label Mar 23, 2023

gciavarrini approved these changes Mar 23, 2023

View reviewed changes

openshift-merge-robot merged commit 258fd2d into parodos-dev:main Mar 23, 2023

		Tibjms tibjms = Mockito.mock(Tibjms.class);
		TibcoWorkFlowTask task = new TibcoWorkFlowTask(tibjms, url, caFile, username, passowrd);

Add a workflow task that sends a message to topic in TIBCO #123

Add a workflow task that sends a message to topic in TIBCO #123

Conversation

ydayagi commented Mar 17, 2023 • edited by openshift-ci bot Loading

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydayagi commented Mar 20, 2023

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

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

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

Choose a reason for hiding this comment

masayag commented Mar 22, 2023

gciavarrini commented Mar 22, 2023

pkliczewski commented Mar 23, 2023 • edited Loading

pkliczewski commented Mar 23, 2023

gciavarrini left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 23, 2023

ydayagi commented Mar 17, 2023 •

edited by openshift-ci bot

Loading

pkliczewski commented Mar 23, 2023 •

edited

Loading