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

Throw error to force error status in workflow UI #135

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gabriel-farache
Copy link
Collaborator

@gabriel-farache gabriel-farache commented Mar 19, 2024

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

Currently, when an error occurs in the workflows, their status in the UI is completed which is not really a good feedback; it should be error instead to reflect the real state of the workflow.

We can hack it, based on https://github.com/tiagodolphine/backstage-orchestrator-workflows/blob/main/workflows/wait-or-error.sw.yaml, to throw an error, if something is wrong in the workflow, in order to force the status to error and better reflects the real state of the workflow

@gabriel-farache gabriel-farache marked this pull request as draft March 19, 2024 09:16
@gabriel-farache gabriel-farache force-pushed the feat/end_in_error branch 3 times, most recently from 0ad23be to 1ad8d77 Compare March 22, 2024 08:28
@rgolangh
Copy link
Collaborator

rgolangh commented May 2, 2024

We can expect the kogito runtime to mark the process instance as failed when this is resolved apache/incubator-kie-kogito-runtimes#3475

@rgolangh
Copy link
Collaborator

rgolangh commented May 2, 2024

this would mean we would need a slight definition change to make that happen

states:
  name: endWithError
  metadata:
     errorMessage: '.somekey'
  end: true

@gabriel-farache gabriel-farache force-pushed the feat/end_in_error branch 2 times, most recently from d7bf7a0 to f65060c Compare May 23, 2024 13:30
@gabriel-farache
Copy link
Collaborator Author

gabriel-farache commented May 23, 2024

@rgolangh With apache/incubator-kie-kogito-runtimes#3475 / apache/incubator-kie-kogito-runtimes#3491 it works
I had to use the latest swf nightly builder image to be able to use the feature, hence updating the DEV dockerfile to latest and using matching quarkus extensions

But I guess we do not want to merge that until we have the fix available in stable build (cc @pkliczewski @masayag ), maybe @ricardozanini knows when this fix will be embedded

Copy link
Collaborator

@masayag masayag left a comment

Choose a reason for hiding this comment

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

As stated, let's merge after there is an available stable build with the required enhancement.

@pkliczewski
Copy link
Contributor

@gabriel-farache apache/incubator-kie-kogito-runtimes#3475 was fixed and now we can set workflow end status

@gabriel-farache
Copy link
Collaborator Author

@gabriel-farache apache/incubator-kie-kogito-runtimes#3475 was fixed and now we can set workflow end status

@pkliczewski Yes, I know, that is what I wrote in my previous comment.
But fixed does not mean it's on a stable release of the swf-builder: latest is more than 4 month, CR1 dates back from 15th of April whereas the issue was merged ~3rd of May

@pkliczewski
Copy link
Contributor

@gabriel-farache there is new stable build release 2 days ago and @masayag will create stable branch soon and use this build. I think we have everything we need to use it.

@rgolangh
Copy link
Collaborator

@gabriel-farache can you resume this work?

@@ -1,4 +1,4 @@
FROM quay.io/kiegroup/kogito-swf-builder-nightly:main-2024-04-08 AS builder
FROM quay.io/kiegroup/kogito-swf-builder-nightly:latest AS builder
Copy link
Collaborator

Choose a reason for hiding this comment

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

according to the security recommendations, it isn't advised to use the latest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree but this is the only image with the feature in it: apache/incubator-kie-kogito-runtimes#3491 was merged on may 3rd but the most recent image with a fixed tag dates from April 28th

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gabriel-farache
Copy link
Collaborator Author

@rgolangh We should close it IMO, completion and error are related to the workflow runtime execution, not the status of what the workflow is doing
There should be a separated status for that, I think there was some issue/discussion about that but I can't recall.

Plus, Gary stated in his latest comment https://issues.redhat.com/browse/FLPATH-1127 that it's fine

@gabriel-farache
Copy link
Collaborator Author

nevermind my above comment, it appears that I made some change based on apache/incubator-kie-kogito-runtimes#3475 that I completly forgot about and that this is the baseline to manage workflow "purpose" failure

@gabriel-farache
Copy link
Collaborator Author

@rgolangh

@gabriel-farache can you resume this work?

I only need a swf builder image with a fixed tag

@@ -1,4 +1,4 @@
FROM quay.io/kiegroup/kogito-swf-builder-nightly:main-2024-04-08 AS builder
FROM quay.io/kiegroup/kogito-swf-builder-nightly:latest AS builder
Copy link
Collaborator

Choose a reason for hiding this comment

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

latest isn't recommended by konflux, let's pin to a specific version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes but this is for dev and IMO, for dev purposes it's OK to use the latest to test changes, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Somehow I remembered that konflux has no-latest-tag tolerance. if that isn't the case, that's fine.
but giving it another thought - we won't be able to know against which builder image the workflow was created.
that should provide us some information when debugging issues or when trying to reproduce one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that should provide us some information when debugging issues or when trying to reproduce one.

the dev image should only be used when developing something on the workflow, so that does no apply here

we won't be able to know against which builder image the workflow was created.

When someone is developing (s)he knows what's going on

Copy link
Collaborator

@rgolangh rgolangh Jul 4, 2024

Choose a reason for hiding this comment

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

I don't mind chaning that dockedrfile, it is not in use by konflux or the the github action. Just looking more closely I see that there is no meaningful difference to the workflow-builder.Dockerfile other than the builder image. In that case why not remove this file altogether and use a build-arg if you want some latest? or even create a make rule for building for devlopers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There used to be differences between the two. If this is no longer the case (when we had the extensions included in the build image of the d/s builder image).
Wouldn't the developer will need to have a dockerfile, even with argfile? where do you recommend to have that file for him? (talking about upstream contributor/developer)

@gabriel-farache gabriel-farache force-pushed the feat/end_in_error branch 6 times, most recently from b88693f to 02d80e5 Compare July 5, 2024 13:10
Signed-off-by: gabriel-farache <gfarache@redhat.com>
@rgolangh
Copy link
Collaborator

rgolangh commented Aug 1, 2024

can you please rephrase the cover message for the PR?

@rgolangh
Copy link
Collaborator

rgolangh commented Aug 1, 2024

can you add an MTA e2e test that will test this failure mode?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants