Making taskrun logs interactive. #491

16yuki0702 · 2019-12-03T01:45:19Z

Changes

Related issue is #263

adding interactive menu to taskrun logs.

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

taskrun logs will be interactive

Related issue is tektoncd#263

tekton-robot · 2019-12-03T01:45:34Z

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.

16yuki0702 · 2019-12-03T01:50:37Z

pkg/helper/taskrun/list/list.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package list


I wanted to define it pkg/helper/taskrun/taskrun.go at first,
but pkg/helper/taskrun/taskrun.go already imported github.com/tektoncd/cli/pkg/cmd/taskrun to using taskrun's logreader, logwriter.

so this is a workaround to avoid cycle importing.
(I think it would be better extracting logreader, logwriter to common utility packages later!)

danielhelfand · 2019-12-03T01:55:02Z

/ok-to-test

tekton-robot · 2019-12-03T01:58:13Z

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/helper/options/logs.go	78.6%	80.0%	1.4
pkg/helper/taskrun/list/list.go	Do not exist	87.5%

16yuki0702 · 2019-12-03T02:22:50Z

/retest

16yuki0702 · 2019-12-03T04:17:10Z

/retest

danielhelfand · 2019-12-03T15:17:15Z

@16yuki0702 Issues with integration tests are unrelated to your changes. We're digging into it now.

danielhelfand · 2019-12-03T16:21:56Z

/test pull-tekton-cli-integration-tests

danielhelfand · 2019-12-03T20:10:09Z

pkg/helper/options/logs_test.go

@@ -143,6 +172,9 @@ func TestLogOptions_Ask(t *testing.T) {
 			if opts.PipelineRunName != tp.want.PipelineRunName {
 				t.Errorf("unexpected PipelineRunName")
 			}
+			if opts.TaskrunName != tp.want.TaskrunName {
+				t.Errorf("unexpected TaskRunName")


Suggested change

t.Errorf("unexpected TaskRunName")

t.Errorf("Unexpected TaskRun name")

danielhelfand · 2019-12-03T20:10:30Z

pkg/helper/taskrun/list/list_test.go

+		t.Run(tp.name, func(t *testing.T) {
+			got, err := GetAllTaskRuns(tp.params, metav1.ListOptions{}, 5)
+			if err != nil {
+				t.Errorf("unexpected Error")


Suggested change

t.Errorf("unexpected Error")

t.Errorf("Unexpected error")

danielhelfand · 2019-12-03T20:11:29Z

pkg/cmd/taskrun/logs_test.go

+				}
+			} else {
+				if err != nil {
+					t.Errorf("unexpected Error")


Suggested change

t.Errorf("unexpected Error")

t.Errorf("Unexpected error")

danielhelfand · 2019-12-03T20:27:34Z

pkg/helper/options/logs_test.go

+			prompt: htest.PromptTest{
+				CmdArgs: []string{},
+				Procedure: func(c *goexpect.Console) error {
+					if _, err := c.ExpectString("Select taskrun :"); err != nil {


Suggested change

if _, err := c.ExpectString("Select taskrun :"); err != nil {

if _, err := c.ExpectString("Select taskrun:"); err != nil {

Can you remove the space before : and also in the associated code in Ask? There may also be other tests that are affected.

16yuki0702 · 2019-12-04T10:13:09Z

@danielhelfand

@16yuki0702 Issues with integration tests are unrelated to your changes. We're digging into it now.

Thank you for addressing it and reviews!
I fixed all of them 🙏

tekton-robot · 2019-12-04T10:23:16Z

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/helper/options/logs.go	78.6%	80.0%	1.4
pkg/helper/taskrun/list/list.go	Do not exist	87.5%

16yuki0702 · 2019-12-04T10:59:04Z

/retest

16yuki0702 · 2019-12-04T11:24:46Z

/test pull-tekton-cli-unit-tests

16yuki0702 · 2019-12-04T11:53:35Z

Umm... where is error detail page 🤔
I run go test ./pkg/cmd/pipelineresource/ locally, but all tests are passed 🤔

danielhelfand · 2019-12-04T12:01:29Z

@16yuki0702 The two unit tests that are failing are notoriously flaky running as part of the CI. We're going to be looking into the cause soon, but your changes aren't the issue.

/test pull-tekton-cli-unit-tests

16yuki0702 · 2019-12-04T12:06:55Z

@danielhelfand

@16yuki0702 The two unit tests that are failing are notoriously flaky running as part of the CI. We're going to be looking into the cause soon, but your changes aren't the issue.

Oh... I got it, thanks!!

danielhelfand · 2019-12-04T12:28:30Z

/test pull-tekton-cli-unit-tests

danielhelfand

Thanks @16yuki0702!

vdemeester

/lgtm

tekton-robot · 2019-12-04T15:24:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielhelfand, 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

Making taskrun logs interactive.

de4cb22

Related issue is tektoncd#263

tekton-robot requested review from hrishin and sthaha December 3, 2019 01:45

googlebot added the cla: yes label Dec 3, 2019

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 3, 2019

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 3, 2019

16yuki0702 commented Dec 3, 2019

View reviewed changes

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 Dec 3, 2019

danielhelfand reviewed Dec 3, 2019

View reviewed changes

Correcting interactive menu string and testing error messages

964b038

danielhelfand approved these changes Dec 4, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2019

vdemeester approved these changes Dec 4, 2019

View reviewed changes

tekton-robot assigned vdemeester Dec 4, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2019

tekton-robot merged commit f9678c1 into tektoncd:master Dec 4, 2019

16yuki0702 deleted the make_taskrun_logs_interactive branch December 9, 2019 01:30

danielhelfand mentioned this pull request Dec 16, 2019

taskrun logs should have an interactive menu #263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making taskrun logs interactive. #491

Making taskrun logs interactive. #491

16yuki0702 commented Dec 3, 2019

tekton-robot commented Dec 3, 2019

16yuki0702 Dec 3, 2019 •

edited

Loading

danielhelfand commented Dec 3, 2019

tekton-robot commented Dec 3, 2019

16yuki0702 commented Dec 3, 2019

16yuki0702 commented Dec 3, 2019

danielhelfand commented Dec 3, 2019 •

edited

Loading

danielhelfand commented Dec 3, 2019

danielhelfand Dec 3, 2019

danielhelfand Dec 3, 2019

danielhelfand Dec 3, 2019

danielhelfand Dec 3, 2019 •

edited

Loading

16yuki0702 commented Dec 4, 2019

tekton-robot commented Dec 4, 2019

16yuki0702 commented Dec 4, 2019

16yuki0702 commented Dec 4, 2019

16yuki0702 commented Dec 4, 2019

danielhelfand commented Dec 4, 2019

16yuki0702 commented Dec 4, 2019

danielhelfand commented Dec 4, 2019

danielhelfand left a comment

vdemeester left a comment

tekton-robot commented Dec 4, 2019

	t.Errorf("unexpected TaskRunName")
	t.Errorf("Unexpected TaskRun name")

	if _, err := c.ExpectString("Select taskrun :"); err != nil {
	if _, err := c.ExpectString("Select taskrun:"); err != nil {

Making taskrun logs interactive. #491

Making taskrun logs interactive. #491

Conversation

16yuki0702 commented Dec 3, 2019

Changes

Submitter Checklist

Release Notes

tekton-robot commented Dec 3, 2019

16yuki0702 Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

danielhelfand commented Dec 3, 2019

tekton-robot commented Dec 3, 2019

16yuki0702 commented Dec 3, 2019

16yuki0702 commented Dec 3, 2019

danielhelfand commented Dec 3, 2019 • edited Loading

danielhelfand commented Dec 3, 2019

danielhelfand Dec 3, 2019

Choose a reason for hiding this comment

danielhelfand Dec 3, 2019

Choose a reason for hiding this comment

danielhelfand Dec 3, 2019

Choose a reason for hiding this comment

danielhelfand Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

16yuki0702 commented Dec 4, 2019

tekton-robot commented Dec 4, 2019

16yuki0702 commented Dec 4, 2019

16yuki0702 commented Dec 4, 2019

16yuki0702 commented Dec 4, 2019

danielhelfand commented Dec 4, 2019

16yuki0702 commented Dec 4, 2019

danielhelfand commented Dec 4, 2019

danielhelfand left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Dec 4, 2019

16yuki0702 Dec 3, 2019 •

edited

Loading

danielhelfand commented Dec 3, 2019 •

edited

Loading

danielhelfand Dec 3, 2019 •

edited

Loading