kind/feat: Surface artifacts through termination message #7714
Conversation
|
Skipping CI for Draft Pull Request. |
tekton-robot
added
do-not-merge/work-in-progress
tekton-robot
added
the
size/XL
|
/kind feat |
|
@ericzzzzzzz: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/kind feature |
tekton-robot
added
the
kind/feature
|
The following is the coverage report on the affected files.
|
ericzzzzzzz
force-pushed
the
surface-artifacts-through-termination-message
branch
from
16ed163
to
7443e91
Compare
tekton-robot
added
size/L
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
size/XL
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
ericzzzzzzz
changed the title
tekton-robot
added
release-note-none
|
The following is the coverage report on the affected files.
|
tekton-robot
added
release-note
|
Some questions about the pr:
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
ericzzzzzzz
force-pushed
the
surface-artifacts-through-termination-message
branch
from
b8abe73
to
1ea4d6e
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
ericzzzzzzz
force-pushed
the
surface-artifacts-through-termination-message
branch
from
1ea4d6e
to
285f5e6
Compare
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
ericzzzzzzz
force-pushed
the
surface-artifacts-through-termination-message
branch
from
285f5e6
to
de45f72
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
hi @afrittoli I updated the pr according your suggestions! Please take another look, thank you for the review ! |
ericzzzzzzz
force-pushed
the
surface-artifacts-through-termination-message
branch
from
7b0282b
to
7e2768c
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| - name: artifacts-consumer-python | ||
| image: python:latest | ||
| script: | | ||
| #!/usr/bin/env python3 | ||
| import json | ||
| data = json.loads('$(steps.artifacts-producer.outputs)') | ||
| print(data[0]['uri']) |
| dataBytes, err := os.ReadFile(e.Command[0]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fileContent := string(dataBytes) | ||
| v, err := replaceValue(artifactref.StepArtifactRegex, fileContent, stepDir, getArtifactValues) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if v != fileContent { | ||
| temp, err := writeToTempFile(v) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| e.Command = []string{temp.Name()} | ||
| } |
There was a problem hiding this comment.
According to the coverage report, this section is not well covered by unit tests, but we have E2E coverage for this, so it should be fine.
| @@ -281,6 +282,15 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL | |||
| logger.Errorf("termination message could not be parsed as JSON: %v", err) | |||
| merr = multierror.Append(merr, err) | |||
| } else { | |||
| for _, r := range results { | |||
| if r.ResultType == result.ArtifactsResultType { | |||
| if err := json.Unmarshal([]byte(r.Value), &as); err != nil { | |||
There was a problem hiding this comment.
NIT: in all other error cases, we have a logger.Errorf message, should we have one here too?
There was a problem hiding this comment.
sure I'll do it !
Fixes: #7717
TaskRunstatus stepState.Changes
$(step.artifacts.path)to/tekton/steps/<container-name>/artifacts/provenance.jsonin the Step container at runtime, and users can write artifacts provenance data to the path. Details about the data structure https://github.com/tektoncd/community/blob/main/teps/0147-tekton-artifacts-phase1.md#artifact-provenance-datatermination message, provenance data is written to termination message and populates tostepState.$(steps.<the-name-of-step>.outputs)--> outputs are artifact values from the first artifact$(steps.<the-name-of-step>.outputs.<artifact-name>)--> outputs for a specific artifact$(steps.<the-name-of-step>.inputs)--> inputs are artifact values from the first artifact$(steps.<the-name-of-step>.inputs.<artifact-name>)--> inputs for a specific artifact$(step.artifacts.path)and$(steps.<step-name>.outputs/inputs.<artifact-category-name>)can only be used in step.Script, step.Env, step.Args, steps.Common with enable-artifacts feature is set totrue$(steps.<step-name>.outputs/inputs.<artifact-category-name>)is used, we will read the script file and re-write container command with script.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
/kind feat