Skip to content
This repository has been archived by the owner on Jul 23, 2024. It is now read-only.

combine task status with its checker #247

Merged
merged 1 commit into from
Apr 19, 2023
Merged

combine task status with its checker #247

merged 1 commit into from
Apr 19, 2023

Conversation

RichardW98
Copy link
Contributor

@RichardW98 RichardW98 commented Apr 17, 2023 •
edited
Loading

FLPATH-299

UI shows task completed even its checker is still running.
this pr will display task in_progress if its checker is not completed.

example: checker waiting jira approval
Screenshot 2023-04-17 at 5 25 01 PM

getStatus response:

{
  "workFlowExecutionId": "27912bce-bcdf-4d32-af32-5587327412c8",
  "workFlowName": "ocpOnboardingWorkFlow",
  "status": "IN_PROGRESS",
  "works": [
    {
      "name": "workFlowA",
      "type": "WORKFLOW",
      "status": "IN_PROGRESS",
      "works": [
        {
          "name": "jiraTicketCreationWorkFlowTask",
          "type": "TASK",
          "status": "IN_PROGRESS"
        },
        {
          "name": "notificationWorkFlowTask",
          "type": "TASK",
          "status": "PENDING"
        },
        {
          "name": "jiraTicketEmailNotificationWorkFlowTask",
          "type": "TASK",
          "status": "PENDING"
        }
      ]
    },
    {
      "name": "workFlowB",
      "type": "WORKFLOW",
      "status": "PENDING"
    }
  ]
}

@openshift-ci openshift-ci bot requested review from lshannon and masayag April 17, 2023 21:25
@RichardW98 RichardW98 requested a review from anludke April 17, 2023 21:26
pkliczewski
pkliczewski reviewed Apr 18, 2023
View reviewed changes
...src/test/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceDelegateTest.java Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it makes sense to check for task PENDING status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checker workflow will not have pending status, its execution entity is null if it's not started. Other workflows/tasks will be pending if their executions are null

Copy link
Contributor Author

@RichardW98 RichardW98 Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

in getStatus endpoint, we don't return checker status. But I am planning to include it in the return later on and show checker status separately in a popup/window in UI

eloycoto
eloycoto reviewed Apr 18, 2023
View reviewed changes
...ice/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceDelegate.java
|| WorkFlowStatus.IN_PROGRESS.equals(workExecution.getStatus()) ? ParodosWorkStatus.PENDING
: ParodosWorkStatus.valueOf(workExecution.getStatus().name());
ParodosWorkStatus workStatus;
if (workExecution == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the logic here has some redundency, I have fixed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

...ice/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceDelegate.java
workStatus = Optional
.ofNullable(workFlowRepository.findFirstByMasterWorkFlowExecutionAndWorkFlowDefinitionId(
workFlowExecution.getMasterWorkFlowExecution(),
workFlowTaskDefinition
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you use the service instead of the repository, definitions are in another package, and service should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just found there will be cyclic dependency if I use workflowService here, as workflowDelegate is also dependent of workflowService: https://github.com/RichardW98/parodos/blob/combine-task-checker-status/workflow-service/src/main/java/com/redhat/parodos/workflow/execution/service/WorkFlowServiceImpl.java#L72

Copy link
Contributor Author

Choose a reason for hiding this comment

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

personally I think workflowService should depend on its delegate but not in reverse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a ticket in our board to rearrange dependencies: https://issues.redhat.com/browse/FLPATH-290

@RichardW98 RichardW98 force-pushed the combine-task-checker-status branch 3 times, most recently from b5d9891 to b54262d Compare April 18, 2023 14:48
eloycoto
eloycoto reviewed Apr 18, 2023
View reviewed changes
.../main/java/com/redhat/parodos/workflow/definition/service/WorkFlowDefinitionServiceImpl.java
@@ -247,6 +247,7 @@ private void getWorksFromWorkDefinition(List<WorkFlowWorkDefinition> workFlowWor
return;
}
responseDTOs.add(WorkDefinitionResponseDTO.fromWorkFlowTaskDefinition(wdt.get()));
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this break is not needed at all, n?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need it to avoid the errors (workflow with id not found) when app starts

Copy link
Contributor Author

@RichardW98 RichardW98 Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

in java, if no break then it continues to the case after it regardless if matches

@RichardW98 RichardW98 force-pushed the combine-task-checker-status branch 2 times, most recently from d406ced to 5f8a95d Compare April 18, 2023 17:03
@RichardW98 RichardW98 marked this pull request as draft April 19, 2023 03:32
@RichardW98 RichardW98 marked this pull request as ready for review April 19, 2023 03:45
@openshift-ci openshift-ci bot requested a review from ydayagi April 19, 2023 03:45
pkliczewski
pkliczewski reviewed Apr 19, 2023
View reviewed changes
Copy link
Collaborator

@pkliczewski pkliczewski left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 19, 2023
@masayag
Copy link
Collaborator

masayag commented Apr 19, 2023

/approve

@openshift-ci
Copy link

openshift-ci bot commented Apr 19, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: masayag

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 9e751d9 into parodos-dev:main Apr 19, 2023
@RichardW98 RichardW98 deleted the combine-task-checker-status branch May 8, 2023 15:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pkliczewski pkliczewski pkliczewski left review comments

@lshannon lshannon Awaiting requested review from lshannon

@masayag masayag Awaiting requested review from masayag

@anludke anludke Awaiting requested review from anludke

@eloycoto eloycoto Awaiting requested review from eloycoto

@ydayagi ydayagi Awaiting requested review from ydayagi

Assignees

@pkliczewski pkliczewski

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RichardW98 @masayag @eloycoto @pkliczewski @openshift-merge-robot