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

fix: Mark resource & data template report-outputs-completed: "true" #12544

Merged

Conversation

shuangkun
Copy link
Member

@shuangkun shuangkun commented Jan 18, 2024

I run two example(use latest image):
https://github.com/argoproj/argo-workflows/blob/main/test/e2e/testdata/resource-templates/outputs.yaml
https://github.com/argoproj/argo-workflows/blob/main/test/e2e/testdata/data-transformation.yaml
I find the workflow succeed but the workflowtaskresult have two is not marked true.
The two is resource and data template. So I want to mark resource and data report-outputs-completed true.

tianshuangkun@B-QGHAMD6M-2124 myworkflow % argo list
NAME                        STATUS      AGE   DURATION   PRIORITY
data-transformation-ff287   Succeeded   2h    2h         0
outputs-fww7h               Succeeded   6h    6h         0
tianshuangkun@B-QGHAMD6M-2124 myworkflow % kubectl get workflowtaskresults -o yaml | grep workflows.argoproj.io/report-outputs-completed
      workflows.argoproj.io/report-outputs-completed: "true"
      workflows.argoproj.io/report-outputs-completed: "true"
      workflows.argoproj.io/report-outputs-completed: "true"
      workflows.argoproj.io/report-outputs-completed: "true"
      workflows.argoproj.io/report-outputs-completed: "true"
      workflows.argoproj.io/report-outputs-completed: "false"
      workflows.argoproj.io/report-outputs-completed: "true"
      workflows.argoproj.io/report-outputs-completed: "false"
tianshuangkun@B-QGHAMD6M-2124 myworkflow %

Motivation

Modifications

Verification

@shuangkun shuangkun marked this pull request as draft January 18, 2024 12:28
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun shuangkun force-pushed the fix/PatchTaskResultCompleted branch 2 times, most recently from de7fb0c to 52bddf0 Compare January 18, 2024 13:55
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun shuangkun marked this pull request as ready for review January 18, 2024 14:53
@juliev0 juliev0 self-assigned this Jan 21, 2024
@juliev0 juliev0 added the prioritized-review For members of the Sustainability Effort label Jan 23, 2024
@juliev0
Copy link
Contributor

juliev0 commented Jan 24, 2024

Great job catching this ahead of another Issue that found it! And resolving it as well.

@juliev0
Copy link
Contributor

juliev0 commented Jan 24, 2024

@Garett-MacGowan will you also review this one?

@Garett-MacGowan
Copy link
Contributor

Once comments are resolved, LGTM.

Signed-off-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@Garett-MacGowan
Copy link
Contributor

Garett-MacGowan commented Jan 25, 2024

Just to be clear, we were already initializing outputs (creating a task result) somewhere in order to get workflows.argoproj.io/report-outputs-completed: "false", right? Where was this happening? If its the wait container running in the main container, why does the defer not run? Do we actually need to initialize outputs here? Aren't we double initializing?

Edit: I should have looked deeper before commenting. This was a shower thought.

@Garett-MacGowan
Copy link
Contributor

Garett-MacGowan commented Jan 25, 2024

Just to be clear, we were already initializing outputs (creating a task result) somewhere in order to get workflows.argoproj.io/report-outputs-completed: "false", right? Where was this happening? If its the wait container running in the main container, why does the defer not run? Do we actually need to initialize outputs here? Aren't we double initializing?

Nevermind, ReportOutputs does an upsert, so its necessary to initialize here. LGTM

Signed-off-by: shuangkun <tsk2013uestc@163.com>
@juliev0
Copy link
Contributor

juliev0 commented Jan 26, 2024

Just to be clear, we were already initializing outputs (creating a task result) somewhere in order to get workflows.argoproj.io/report-outputs-completed: "false", right? Where was this happening? If its the wait container running in the main container, why does the defer not run? Do we actually need to initialize outputs here? Aren't we double initializing?

Edit: I should have looked deeper before commenting. This was a shower thought.

I can relate to the "shower thought" :)

@juliev0 juliev0 merged commit a155877 into argoproj:main Jan 26, 2024
27 checks passed
@agilgur5 agilgur5 added the area/controller Controller issues, panics label Jan 28, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 4, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 20, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 27, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 28, 2024
…goproj#12544)

Signed-off-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: Isitha Subasinghe <isubasinghe@student.unimelb.edu.au>
@agilgur5 agilgur5 changed the title fix: Mark resource && data template report-outputs-completed true fix: Mark resource & data template report-outputs-completed: "true" Jul 8, 2024
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.

4 participants