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.

Sign up for GitHub

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

Jump to bottom

Make pipelinerun logs interactive. #432

Merged
merged 1 commit into from
Nov 27, 2019
Merged

Make pipelinerun logs interactive. #432

merged 1 commit into from
Nov 27, 2019

Conversation

16yuki0702
Copy link
Member

@16yuki0702 16yuki0702 commented Nov 2, 2019 •
edited
Loading

Changes

Related issues are #263 #414

This is a kind of suggesion PR.
To introduce interactive menu to task and taskrun logs, I did belows.

  1. Extract reusable struct and methods to other packages.
  2. Make Pipelinerun logs interactive as a first step.

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)
  • Includes docs (if user facing)
  • Regenerate the manpages and docs with make docs and make man if needed.
  • Run the code checkers with make check
  • Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

pipelinerun logs will be interactive

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 2, 2019
@tekton-robot
Copy link
Contributor

Hi @16yuki0702. 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 tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 2, 2019
@danielhelfand
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 2, 2019
@tekton-robot
Copy link
Contributor

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/cmd/pipeline/logs.go 83.3% 86.3% 3.0
pkg/cmd/pipelinerun/logs.go 92.6% 85.0% -7.6
pkg/helper/pipeline/pipeline.go Do not exist 0.0%
pkg/helper/pipelinerun/pipelinerun.go Do not exist 0.0%

@16yuki0702
Copy link
Member Author

/retest

@tekton-robot
Copy link
Contributor

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/cmd/pipeline/logs.go 83.3% 86.3% 3.0
pkg/cmd/pipelinerun/logs.go 92.6% 85.0% -7.6
pkg/helper/pipeline/pipeline.go Do not exist 0.0%
pkg/helper/pipelinerun/pipelinerun.go Do not exist 0.0%

@tekton-robot
Copy link
Contributor

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/cmd/pipeline/logs.go 83.3% 86.3% 3.0
pkg/cmd/pipelinerun/logs.go 92.6% 85.0% -7.6
pkg/helper/pipeline/pipeline.go Do not exist 0.0%
pkg/helper/pipelinerun/pipelinerun.go Do not exist 0.0%

@16yuki0702 16yuki0702 force-pushed the make_pipelinerun_logs_interactive branch from deddb8d to d4e24d8 Compare November 5, 2019 05:42
@tekton-robot
Copy link
Contributor

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/cmd/pipeline/logs.go 83.3% 86.3% 3.0
pkg/cmd/pipelinerun/logs.go 92.6% 85.0% -7.6
pkg/helper/pipeline/pipeline.go Do not exist 0.0%
pkg/helper/pipelinerun/pipelinerun.go Do not exist 0.0%

@tekton-robot
Copy link
Contributor

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/cmd/pipeline/logs.go 83.3% 86.3% 3.0
pkg/cmd/pipelinerun/logs.go 92.6% 85.0% -7.6
pkg/helper/pipeline/pipeline.go Do not exist 0.0%
pkg/helper/pipelinerun/pipelinerun.go Do not exist 0.0%

piyush-garg
piyush-garg approved these changes Nov 7, 2019
View reviewed changes
Copy link
Contributor

@piyush-garg piyush-garg left a comment

Choose a reason for hiding this comment

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

/lgtm

One comment: can you please squash the two commits into one. Thanks

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2019
@16yuki0702 16yuki0702 force-pushed the make_pipelinerun_logs_interactive branch from 690792d to 3537d30 Compare November 8, 2019 05:35
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2019
@16yuki0702
Copy link
Member Author

16yuki0702 commented Nov 8, 2019 •
edited
Loading

@piyush-garg Thank you for your review!

One comment: can you please squash the two commits into one. Thanks

I squashed the pr, and added test cases 👍

@piyush-garg
Copy link
Contributor

@16yuki0702 Thanks for the update. Can you please add an interactive test case also in pkg/cmd/pipelinerun/log_test.go that will be great

@tekton-robot
Copy link
Contributor

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/cmd/pipeline/logs.go 83.3% 86.3% 3.0
pkg/cmd/pipelinerun/logs.go 92.6% 85.0% -7.6
pkg/helper/options/logs.go Do not exist 0.0%
pkg/helper/pipeline/pipeline.go Do not exist 81.8%
pkg/helper/pipelinerun/pipelinerun.go Do not exist 87.5%

danielhelfand
danielhelfand reviewed Nov 8, 2019
View reviewed changes
pkg/cmd/pipelinerun/logs.go
return fmt.Errorf("No pipelineruns found")
}

if len(prs) == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test case for len(prs) == 1?

danielhelfand
danielhelfand reviewed Nov 8, 2019
View reviewed changes
pkg/helper/options/logs.go
// See the License for the specific language governing permissions and
// limitations under the License.

package options
Copy link
Member

Choose a reason for hiding this comment

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

Can you add explicit test cases here?

@16yuki0702
Copy link
Member Author

@piyush-garg @danielhelfand Thank for reviews!!
I have no enough time this week to address these, please wait sorry 😓

@danielhelfand
Copy link
Member

@16yuki0702 Thanks and no rush! We appreciate how much you do for tkn.

@tekton-robot
Copy link
Contributor

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/cmd/pipeline/logs.go 83.3% 86.3% 3.0
pkg/cmd/pipelinerun/logs.go 92.6% 92.5% -0.1
pkg/helper/options/delete.go Do not exist 92.9%
pkg/helper/options/logs.go Do not exist 73.3%
pkg/helper/pipeline/pipeline.go Do not exist 81.8%
pkg/helper/pipelinerun/pipelinerun.go Do not exist 87.5%

@16yuki0702
Copy link
Member Author

@piyush-garg @danielhelfand sorry for late 😓, I added the test cases please review it in your free time!

danielhelfand
danielhelfand reviewed Nov 25, 2019
View reviewed changes
pkg/helper/options/logs.go Outdated
}

if err := survey.Ask(qs, &ans, opts.AskOpts); err != nil {
fmt.Println(err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? Wouldn't the error be displayed via the err that is returned?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for review!!
yes, you are right, I think this err will be displayed via returned.
I just tried to match other format like these.

https://github.com/tektoncd/cli/blob/master/pkg/cmd/pipeline/start.go#L231
https://github.com/tektoncd/cli/blob/master/pkg/cmd/pipeline/start.go#L273

if it no need, I want to delete other codes also!

Copy link
Member

@danielhelfand danielhelfand Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, please remove all of them. Thanks for finding the additional ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, please remove all of them. Thanks for finding the additional ones.

Thanks!! I did it 👍

@danielhelfand
Copy link
Member

@piyush-garg @danielhelfand sorry for late 😓, I added the test cases please review it in your free time!

No worries! Just a minor note from me. Otherwise, looks good to me.

@tekton-robot
Copy link
Contributor

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/cmd/pipeline/logs.go 83.3% 86.3% 3.0
pkg/cmd/pipeline/start.go 87.7% 88.4% 0.7
pkg/cmd/pipelinerun/logs.go 92.6% 92.5% -0.1
pkg/helper/options/delete.go Do not exist 92.9%
pkg/helper/options/logs.go Do not exist 78.6%
pkg/helper/pipeline/pipeline.go Do not exist 81.8%
pkg/helper/pipelinerun/pipelinerun.go Do not exist 87.5%

@danielhelfand
Copy link
Member

@16yuki0702 Thanks so much! Promise this is the last thing, and apologies for not mentioning sooner. Can you please just squash your commits?

@16yuki0702
Related issues are tektoncd#263 tektoncd#414
ce0c2a1 
Extract common struct and methods to other packages to reuse.
and make pipelinerun logs interactive.
@16yuki0702 16yuki0702 force-pushed the make_pipelinerun_logs_interactive branch from ac147b5 to ce0c2a1 Compare November 27, 2019 03:53
@16yuki0702
Copy link
Member Author

@danielhelfand

@16yuki0702 Thanks so much! Promise this is the last thing, and apologies for not mentioning sooner. Can you please just squash your commits?

oops... I'm so sorry, just squashed PR 🙏

@tekton-robot
Copy link
Contributor

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/cmd/pipeline/logs.go 83.3% 86.3% 3.0
pkg/cmd/pipeline/start.go 87.7% 88.4% 0.7
pkg/cmd/pipelinerun/logs.go 92.6% 92.5% -0.1
pkg/helper/options/delete.go Do not exist 92.9%
pkg/helper/options/logs.go Do not exist 78.6%
pkg/helper/pipeline/pipeline.go Do not exist 81.8%
pkg/helper/pipelinerun/pipelinerun.go Do not exist 87.5%

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2019
vdemeester
vdemeester approved these changes Nov 27, 2019
View reviewed changes
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 Nov 27, 2019
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielhelfand, piyush-garg, 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:
  • OWNERS [danielhelfand,vdemeester]

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 merged commit 4c772bd into tektoncd:master Nov 27, 2019
@16yuki0702 16yuki0702 deleted the make_pipelinerun_logs_interactive branch November 28, 2019 06:38
danielhelfand added a commit that referenced this pull request Dec 11, 2019
@danielhelfand
New version v0.6.0
4a04458 
#478 | [Daniel Helfand] adding timeout to tkn task start | 2019/11/26-10:59
#469 | [Daniel Helfand] include taskrun failure message in logs | 2019/11/26-11:54
#432 | [16yuki0702] Related issues are #263 #414 | 2019/11/27-03:16
#481 | [Vibhav Bobade] Feature: Set current kubernetes context | 2019/11/27-22:06
#481 | [Vibhav Bobade] Updated docs/man with new feature for setting kubecontext | 2019/11/27-22:06
#484 | [Chmouel Boudjnah] Add a step into the RELEASE_PROCESS documentation | 2019/11/29-08:42
#483 | [Piyush Garg] Fix failure in case of array param having single value | 2019/11/29-08:57
#485 | [Daniel Helfand] minor edits to RELEASE_PROCESS.md | 2019/11/29-09:26
#487 | [Daniel Helfand] 0.5.1 version in README | 2019/12/01-11:02
#486 | [Piyush Garg] Improve message for creating new resource in start | 2019/12/02-01:05
null | [Chmouel Boudjnah] Use serviceAccountName instead of serviceAccount | 2019/12/03-10:11
null | [Daniel Helfand] remove extra space from tr not found error | 2019/12/03-10:42
null | [16yuki0702] Making taskrun logs interactive. | 2019/12/04-09:25
null | [16yuki0702] Correcting interactive menu string and testing error messages | 2019/12/04-09:25
null | [Vincent Demeester] Add golint to the linter 🚐 | 2019/12/04-10:16
null | [Vincent Demeester] Fix golint errors 👨‍🍳 | 2019/12/04-10:16
null | [Piyush Garg] Bump tektoncd/pipeline to version 0.9.0 | 2019/12/04-15:12
null | [Piyush Garg] Fixes to support tekton pipeline 0.9.0 | 2019/12/04-15:12
null | [Daniel Helfand] add pipeline create command | 2019/12/05-09:10
null | [Daniel Helfand] add task create command | 2019/12/05-10:05
null | [Piyush Garg] Bump to tektoncd/pipeline 0.9.1 | 2019/12/09-05:09
null | [Daniel Helfand] add namespace check to pipeline and task create commands | 2019/12/09-08:03
null | [Daniel Helfand] add resource create -f option | 2019/12/09-11:56
null | [Chmouel Boudjnah] Add NFPM (deb/rpm) release to goreleaser | 2019/12/10-13:15
null | [Piyush Garg] Fix input params are not respected | 2019/12/11-06:02
null | [Chmouel Boudjnah] Add debian package | 2019/12/11-09:25

Signed-off-by: Daniel Helfand <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielhelfand danielhelfand danielhelfand approved these changes

@piyush-garg piyush-garg piyush-garg approved these changes

@vdemeester vdemeester vdemeester approved these changes

@hrishin hrishin Awaiting requested review from hrishin

@sthaha sthaha Awaiting requested review from sthaha

Assignees

@vdemeester vdemeester

@piyush-garg piyush-garg

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@16yuki0702 @tekton-robot @danielhelfand @piyush-garg @vdemeester @googlebot