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

Improve the error message on pipeline create #316

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

afrittoli
Copy link
Member

Changes

If the pipeline is not found on pipeline create, it may be due to
a misspelled pipeline name or namespace name. Include both in the
error message to help troubleshooting.

Submitter Checklist

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

See the contribution guide
for more details.

@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 17, 2019
@chmouel
Copy link
Member

chmouel commented Sep 17, 2019

I think #311 is related would be nice if we get this fixed everywhere,

/cc @danielhelfand

@afrittoli
Copy link
Member Author

I think #311 is related would be nice if we get this fixed everywhere,

I agree that it would be nice to fix other error messages, I was a bit lazy - only fixed the one I encountered :P

About #311, I think the list and create commands are a bit different use cases. For the create, tkn looks for a specific pipeline specified via CLI parameters - and it may not find it - thus the error message.

For the list case, we don't expect anything specific, so if we don't get anything we can get a message that nothing was found, but that's for information only. To be honest my preference there would be to get no message at all, or if we do get a message get it on stderr - so that it's possible to reliably parse the stdout if needed.

@danielhelfand
Copy link
Member

danielhelfand commented Sep 18, 2019

I think #311 should have a completely different error message that is specific to the namespace. This is more specific to the pipeline name not existing in a valid namespace, but I do think it more clearly communicates the issue to the user.

@chmouel
Copy link
Member

chmouel commented Sep 20, 2019

let me know if that's good to go for you guys ?

@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 20, 2019
@afrittoli
Copy link
Member Author

I've been looking at different commands, and the errors returned a bit mixed up in style:

On pipeline commands:

  • Start: only custom error (the one fixed in this PR)
  • Delete: custom error + original client error
  • Describe: original client error only
  • Log: original client error only
  • List: log a custom error and return the original error

On task commands:

  • Delete: original client error
  • List: log a custom error and return the original error

There are more commands for other resources too, I did not check them all; but it looks to me as if it would be best to agree on a common approach for the error message returned by commands, and then align them all. We could have one shared function used for logging for instance.

My proposal would be:

  • normally only print a custom error message, which includes name and namespace of the resource
  • in verbose mode (--verbose|-v) option passed, also print the original tekton client error

@vdemeester vdemeester added this to the 0.4.0 🐱 milestone Sep 24, 2019
If the pipeline is not found on pipeline create, it may be due to
a misspelled pipeline name or namespace name. Include both in the
error message to help troubleshooting.
When creating a pipelinerun out of a pipeline, the cli outputs the
command to follow the pr logs, which is very handy, only it does
not include the namespace. Fixing that.
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2019
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, vdemeester

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2019
@tekton-robot tekton-robot merged commit 1dabbfc into tektoncd:master Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants