Adds pipelinerun/taskrun logs follow mode and refactors logs code #76

hrishin · 2019-06-13T10:21:15Z

Changes

It's difficult to keep up with the progress of pipelinerun/taskrun
execution if user is not able to see progressive/ongoing logs.

This patch adds follow flag to taskrun/pipelinerun commands.
So use could fetch those logs in real-time.

tkn tr logs -f
tkn pr logs -f

Fixes

Display pipelinerun and taskrun logs in realtime using follow mode #75

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)
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

To follow the pipelinerun logs tkn  in real time 
tkn pr logs <name> -f


To follow the taskrun logs tkn  in real time 
tr logs <name> -f

tekton-robot · 2019-06-13T10:21:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hrishin

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~~ [hrishin]

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

vdemeester

Couple of comments, most of them can be done in a follow-up:

We may want to use golang.org/x/xerrors instead of github.com/pkg/errors as this package will be in the standard library in the future — can be done in a follow-up.
pkg/errors should be aliased to clierrors or something to not conflicts with pkg/errors (or if we use xerrors, the conflict goes away 😅 )
pkg/helper/… feels weird to me. pkg/helper/pipelinerun is more of a "watch pipelinerun logs", same goes for pkg/helper/taskrun, … They should move in another package (pkg/watch/pipelinerun) and splited (having the handleEvent extracted and passed as a function, or at least the part the filters the taskruns based on if we are logging it or not) ?
This is gonna need some godoc 👼
There is a need to move some package around and have some sort of guidelines on which packages are fine to import from wihch (cf my comment about pkg/cmd, …)

pkg/helper/pods/container_test.go

vdemeester · 2019-06-13T16:18:44Z

pkg/cmd/pipelinerun/log_reader.go

+	"sync/atomic"
+
+	"github.com/tektoncd/cli/pkg/errors"
+


nit: extra space not needed 😝

vdemeester · 2019-06-13T16:19:12Z

pkg/cmd/pipelinerun/log_reader.go

+func (lr *LogReader) Read() (<-chan Log, <-chan error, error) {
+	tkn := lr.Clients.Tekton
+	pr, err := tkn.TektonV1alpha1().PipelineRuns(lr.Ns).Get(lr.Run, metav1.GetOptions{})
+


nit: extra space not needed 😝

vdemeester · 2019-06-13T16:20:49Z

pkg/cmd/pipelinerun/log_reader.go

+		defer close(errC)
+
+		prTracker := pipelinerun.NewTracker(pr.Name, lr.Ns, lr.Clients.Tekton)
+		//TODO: return error err chan/error


Is this todo still valid ? what does it refer ?

vdemeester · 2019-06-13T16:21:34Z

pkg/cmd/pipelinerun/log_reader.go

+
+}
+
+func (lr *LogReader) readLiveLogs(pr *v1alpha1.PipelineRun) (<-chan Log, <-chan error, error) {


I feel this funtion could be splitted a tiny bit as it is kinda complex. But that could be done in a follow-up

vdemeester · 2019-06-13T16:46:22Z

pkg/errors/logerror.go

+	msg string
+}
+
+func NewWarning(message string) *WarningError {


We should provide a IsWarning function that would "assert errors for behaviour, not type".
Ref: https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully 👼

vdemeester · 2019-06-13T16:46:36Z

pkg/formatted/colors.go

@@ -0,0 +1 @@
+package formatted


better to name it as format?

My ":thinking:" was about the fact that this file is empty :angel:

vdemeester · 2019-06-13T16:52:08Z

pkg/helper/tasks/tasks.go

@@ -0,0 +1,94 @@
+package tasks


shouldn't this package be taskruns ?

vdemeester · 2019-06-13T16:53:45Z

pkg/helper/tasks/tasks.go

+
+import (
+	"github.com/tektoncd/cli/pkg/cli"
+	"github.com/tektoncd/cli/pkg/cmd/taskrun"


Having this import pkg/cmd/* makes me think the LogReader should be extracted in another package. My rational being pkg/cmd/… should import other package, not the other way around 👼

+1 .. these refactors will happen as soon the feature starts to work. It works mostly but doesn't handle edge cases properly.

@sthaha yep, they can even happen in a follow-up 😉

vdemeester · 2019-06-13T16:59:00Z

pkg/logs/logs.go

-}
-
-type Streams struct {
+// TODO: move this to CLI


To the cli package ?

yes, so that Params interface has a Streams() Streams so that tests can fake the stream as bytes.Buffer for both Out and Err

sthaha · 2019-06-14T00:38:34Z

bash_completion.sh

@@ -0,0 +1,676 @@
+# bash completion for tkn                                  -*- shell-script -*-


was this checked in by mistake?

yeah I guess ... 🤔

googlebot · 2019-06-14T00:52:36Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-06-14T11:55:07Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

vdemeester · 2019-06-14T15:28:52Z

/test pull-tekton-cli-integration-tests

hrishin · 2019-06-14T17:43:22Z

/test pull-tekton-cli-integration-tests

Its difficult to keep up with progress of pipelinerun/taskrun execution if user not able to see progressive/ongoing logs. This patch adds follow flag to taskrun/pipelinerun commands. So use could fetch the those logs in realtime. tkn tr logs <name> -f tkn pr logs <name> -f Fixes - tektoncd#75

sthaha · 2019-06-16T23:08:15Z

pkg/cmd/pipelinerun/log_writer.go

+				continue
+			}
+
+			if l.Log == "LOGEOF" {


This isn't true anymore .. Do we have a special "LOGEOF" ? If so, perhaps we should think of passing "events" in a better way .. events: {started, ended..}

This isn't true anymore .

🤔

hrishin · 2019-06-17T06:56:08Z

@sthaha @vdemeester added final changes, could you please review it?

vdemeester

/lgtm
💃

vdemeester · 2019-06-17T07:37:55Z

pkg/cmd/pipelinerun/log_reader.go

+	}
+
+	//Sort taskruns, to display the taskrun logs as per pipeline tasks order
+	ordered := trh.SortTasksBySpecOrder(pl.Spec.Tasks, pr.Status.TaskRuns)


As a follow-up, we may want to move this function in tektoncd/pipeline (like tektoncd/pipeline@8552f8b#diff-ccd231265a8c68ecb3e8bae3a6e565cf). Can be useful for other go packages.

vdemeester · 2019-06-17T07:47:07Z

pkg/cmd/pipelinerun/logs.go

-		Name:    pipelineRunName,
-		Ns:      namespace,
-		Clients: clients,
+func (lo *LogOptions) run() error {


This could be done with a method that doesn't take any cobra related struct (aka LogOptions and other params) ? The end result is the same, so all good to me anyway 😉

tekton-robot requested review from sthaha and vdemeester June 13, 2019 10:21

googlebot added the cla: yes label Jun 13, 2019

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 13, 2019

vdemeester added this to the 0.1.0 🐱 milestone Jun 13, 2019

vdemeester reviewed Jun 13, 2019

View reviewed changes

sthaha reviewed Jun 14, 2019

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jun 14, 2019

hrishin force-pushed the prs-logs-fix branch from 265b299 to 5de935f Compare June 14, 2019 11:55

googlebot added cla: yes and removed cla: no labels Jun 14, 2019

hrishin marked this pull request as ready for review June 14, 2019 12:25

hrishin force-pushed the prs-logs-fix branch from 8855874 to 37033da Compare June 14, 2019 18:24

removes the warning error and adds formatting: new line after everystep

effcac1

sthaha reviewed Jun 16, 2019

View reviewed changes

hrishin added 3 commits June 17, 2019 06:35

fixes seg fault and container error(podinitializing) issue

02d5274

adds colors to log and error messages

9c58c82

adds copyright headers and comments

821f8cb

vdemeester reviewed Jun 17, 2019

View reviewed changes

tekton-robot assigned vdemeester Jun 17, 2019

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

tekton-robot merged commit 1cabe60 into tektoncd:master Jun 17, 2019

hrishin mentioned this pull request Jun 17, 2019

Fix log command tech depts (refactoring) #88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds pipelinerun/taskrun logs follow mode and refactors logs code #76

Adds pipelinerun/taskrun logs follow mode and refactors logs code #76

hrishin commented Jun 13, 2019 •

edited

Loading

tekton-robot commented Jun 13, 2019

vdemeester left a comment

vdemeester Jun 13, 2019

hrishin Jun 14, 2019

vdemeester Jun 13, 2019

hrishin Jun 14, 2019

vdemeester Jun 13, 2019

vdemeester Jun 13, 2019

vdemeester Jun 13, 2019

vdemeester Jun 13, 2019

hrishin Jun 14, 2019

vdemeester Jun 14, 2019

vdemeester Jun 13, 2019

hrishin Jun 14, 2019

vdemeester Jun 13, 2019

sthaha Jun 14, 2019

vdemeester Jun 14, 2019

vdemeester Jun 13, 2019

sthaha Jun 14, 2019

sthaha Jun 14, 2019

hrishin Jun 14, 2019

googlebot commented Jun 14, 2019

googlebot commented Jun 14, 2019

vdemeester commented Jun 14, 2019

hrishin commented Jun 14, 2019

sthaha Jun 16, 2019

hrishin Jun 17, 2019

hrishin commented Jun 17, 2019

vdemeester left a comment

vdemeester Jun 17, 2019

vdemeester Jun 17, 2019


		}

		func (lr LogReader) readLiveLogs(pr v1alpha1.PipelineRun) (<-chan Log, <-chan error, error) {

		@@ -0,0 +1,676 @@
		# bash completion for tkn -- shell-script --

Adds pipelinerun/taskrun logs follow mode and refactors logs code #76

Adds pipelinerun/taskrun logs follow mode and refactors logs code #76

Conversation

hrishin commented Jun 13, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 13, 2019

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 14, 2019

googlebot commented Jun 14, 2019

vdemeester commented Jun 14, 2019

hrishin commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrishin commented Jun 17, 2019

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrishin commented Jun 13, 2019 •

edited

Loading