reflect state of started pipeline/task when using --showlog

[ileixe]

• this change fixes an issue where using --showlog with tkn start pipeline/task does not return a non-zero value when created taskrun/pipelinerun fails.
• refer Return value of tkn pipeline start with --showlog does not depend on state of started pipeline #1820

Changes

task, clustertask and pipeline start command now returns each run status when completed if user specify --show-log. It provides better usability for tkn CLI to be used in Task or CI.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Run the code checkers with make check
• Regenerate the manpages, docs and go formatting with make generated
• Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

task, clustertask and pipeline start command now returns each run status when completed if user specify `--show-log`. It provides better usability for tkn CLI to be used in Task or CI.

[tekton-robot]

Hi @ileixe. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign pradeepitm12 after the PR has been reviewed.
You can assign the PR to them by writing /assign @pradeepitm12 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[ileixe]

make check now emitted unrelated parts:

🐱 running yamllint…
## runs yamllint on all yaml files
✖ YAML Lint failed for /home/ys/Source/Projects/go/tekton/cli/vendor/gopkg.in/yaml.v2/LICENSE.libyaml
✖ end of the stream or a document separator is expected at line 2, column 58:
     ... r original copyright and license:

I'm not sure how to add test for this logic so please let me know if required.

One thing I did not consider well is possible race condition (status is not yet updated from pipeline). If it's possible case, we may have to add loop to wait for completion.

[vdemeester]

/ok-to-test

[ileixe]

For revision 2: I misused wrong API UseTaskRunFrom so changed to GetTaskRunV1beta1, ... To use GetTaskRunV1beta1 in different places, locate it in taskrun.go (It would to be better place, but not sure).

Integration test did show some race condition issue, so I add 10 sec loop to wait for actual completion. I'm not not very inclined to do, so comments are welcome.

[ileixe]

ps) I made a pull request for latest release branch as I'm using it, but if it's right approach I will make a PR from main.

[tekton-robot]

@ileixe: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-tekton-cli-integration-tests	dc29814	link	true	/test pull-tekton-cli-integration-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[ileixe]

The failed integration test TestPipelinesNegativeE2E/Start_Pipeline_Run_using_pipeline_start_command_with_SA_as_'pipelines'_ , TestPipelinesNegativeE2E/Validate_Pipeline_describe_command_in_namespace_testcli-nvl2p_after_PipelineRun_completed_successfully seems not to be related to this change.

I manually ran the first PipelienRun

wheel-build$ tkn task start --filename /home/ys/Source/Projects/go/tekton/cli/test/resources/pipeline.yaml --showlog -p=message=e2e-test -p=filename=output -w=name=shared-data,emptyDir=
TaskRun started: create-file-verify-run-6fd5d
Waiting for logs to be available...


[read] e2e-test

wheel-build$ echo $?

And it succeed well. Not sure why test just have string {Failed }.

Second one also looks strange as the expected string is different from actual one

Expected

Name:        output-pipeline
Namespace:   testcli-nvl2p

Params

 NAME       TYPE     DESCRIPTION   DEFAULT VALUE
 message    string                 hello
 filename   string                 ---

Workspaces

 NAME          DESCRIPTION   OPTIONAL
 shared-data                 false

Tasks

 NAME                TASKREF              RUNAFTER   TIMEOUT   PARAMS
 first-create-file   create-file-verify              ---       message: hello, filename: string

PipelineRuns

 NAME   STARTED         DURATION   STATUS
        2 seconds ago   0s         Failed(InvalidWorkspaceBindings)

Actual

Name:        output-pipeline
Namespace:   testcli-nvl2p

Params

    NAME       TYPE     DESCRIPTION   DEFAULT VALUE
    message    string                 hello
    filename   string                 ---

Workspaces

    NAME          DESCRIPTION   OPTIONAL
    shared-data                 false

Tasks

    NAME                TASKREF              RUNAFTER   TIMEOUT   PARAMS
    first-create-file   create-file-verify              ---       message: hello, filename: string

PipelineRuns

    NAME                        STARTED         DURATION   STATUS
    output-pipeline-run-q6qwt   2 seconds ago   0s         Failed(InvalidWorkspaceBindings)

None of the cases above seems to be related with this change though, let me know if any possible actions are there.

[piyush-garg]

This seems duplicate PR of #1880 any reason for this, the original author seems working on it

[piyush-garg]

cc @avinal ^^^

[ileixe]

Ah I did not aware that he made a PR (so I just picked his branch), but we already talked in #1820 (comment)

[avinal]

It is fine, please go ahead.

[piyush-garg]

@ileixe Are you working on this or is it good to review? Also can you check the failing test please. Thanks

[ileixe]

@piyush-garg Unfortunately, we found this PR lifted the exit problem a bit, but still have unsound behavior in many cases (not exit at all etc..) We've switched tkn-watch to ensure to have valid exit code in time. I'm not sure how CLI side can ensure correctly, so may not work this PR anymore.