Add a Web view #17

apostergiou · 2018-04-06T09:17:25Z

With this we add a web view for the service for monitoring the available jobs.

The feature introduces:

an index page to view all available jobs and relevant information such as start time and state
a show page for each where we can view the job result information alongside the job build logs. The show page provides a web tailf like feature for monitoring the build log as it unfolds.

agis · 2018-04-06T09:41:39Z

job.go

@@ -192,3 +192,16 @@ func (j *Job) String() string {
 		"{project=%s params=%s group=%s id=%s}",
 		j.Project, j.Params, j.Group, j.ID[:7])
 }
+
+func jobPath(buildPath, project, id string) (string, string, error) {


This should be an exported method on Job and have proper documentation. It should also be given a more accurate name.

agis · 2018-04-06T09:52:53Z

job.go

+func jobPath(buildPath, project, id string) (string, string, error) {
+	pPath := filepath.Join(buildPath, project, "pending", id)
+	rPath := filepath.Join(buildPath, project, "ready", id)
+	if _, err := os.Stat(pPath); err == nil {


We do not use this style, break the error check to a new line.

agis · 2018-04-06T09:53:47Z

local_assets.go

+import "net/http"
+
+func staticFs() (http.FileSystem, error) {
+	return http.Dir("public"), nil


I see no need for this to be a method.

True, deleted.

agis · 2018-04-06T09:54:32Z

public/index.html

+
+  <script src="https://unpkg.com/babel-standalone@6/babel.min.js"></script>
+  <script src="https://unpkg.com/react@16/umd/react.production.min.js"></script>
+  <script src="https://unpkg.com/react-dom@16/umd/react-dom.production.min.js"></script>


Maybe we should also embed these? I mean, we do embed foundation.

Good catch, moved inside the head tag.

agis · 2018-04-06T09:55:45Z

public/js/index.js

+  }
+}
+
+ReactDOM.render(<Jobs every={5000} />, JobsRoot)


I believe we can reduce this to 3sec.

Ok, let's try with 3sec.

agis · 2018-04-06T10:02:01Z

testdata/projects/simple-slow/docker-entrypoint.sh

+sleep 30
+touch artifacts/out.txt
+
+exit 0


This directory should be here only if it's actually used in mistry's tests.

Sure, removed.

agis · 2018-04-06T10:02:24Z

types/build_result.go

@@ -23,6 +23,9 @@ type BuildResult struct {

 	// The method by which the build artifacts can be fetched.
 	TransportMethod TransportMethod
+
+	// The job's start time
+	StartTime string


This should be a time.Time.

agis · 2018-04-06T10:03:23Z

types/job_info.go

+	Output    string
+	Log       string
+	State     string
+}


I see many duplicated fields between this and Job. Can we merge this into Job? It seems redundant.

agis · 2018-04-06T10:04:50Z

server.go

+	clients map[*C]bool
+
+	// A connection queue used to track all open connections by their id.
+	cq map[string]bool


All these fields and methods could go into a separate struct, ideally extracted into its own package and exporting its http handlers that one can use selectively.

Extracted into a broker package.

agis · 2018-04-06T10:07:19Z

tail_reader.go

+		} else if err != io.EOF {
+			return n, err
+		}
+		time.Sleep(5 * time.Second)


Can you elaborate on why is this sleep needed?

Thanks for commenting on this. I added sleep here as a way to give up control to the scheduler while developing the feature. Nevertheless, using it here feels like an anti-pattern and doesn't add anything really so I removed it.

apostergiou · 2018-04-06T13:19:07Z

We should decide when to kill the goroutine that utilizes Tailer. As it stands, the goroutine behaves like tail, reading forever that is, which in our case can and should be avoided. When the job is ready the build log is considered closed, so we should stop tailing it.

On a similar note, as far as the front end is concerned, we could improve the view by updating it when the job is ready. Short polling is a good choice for this.

agis · 2018-04-12T11:32:43Z

This PR should also update the relevant README section.

agis

As we discussed, this should be revamped soon. We should open another issue for it with all the ideas we've discussed.

agis · 2018-04-17T09:14:31Z

README.md

@@ -16,7 +16,7 @@ Features include:
 - caching and reusing build results
 - efficient use of disk space due to copy-on-write semantics
 - a simple JSON API for interacting with the service
- ([wip](https://github.com/skroutz/mistry/pull/17)) a web view for inspecting the progress and result of builds
+- a web view for inspecting the progress and result of builds


This needs a rebase. I slightly updated this point.

True, fixed.

agis · 2018-04-17T09:19:07Z

job.go

+	pPath := filepath.Join(path, project, "pending", id)
+	rPath := filepath.Join(path, project, "ready", id)
+	_, err := os.Stat(pPath)
+	if err == nil {


What if error is non nil but also not a "not exist" error?

It will return the error.

You're right, nvm.

agis · 2018-04-17T09:21:14Z

job.go

+	if err == nil {
+		return "ready", nil
+	}
+	return "", err


I don't like the fact that we may return an empty string and no error. What would such a case indicate?

Correction: we do return a non-nil error here, but still, what does an empty string and a "ready path doesn't exist" error means?

Thanks for noticing, maybe we could return a custom error then in which we have concatenated any errors with a meaningful message, e.g. "state not found for job %s error %v"

A simple "job %s not found error", j I think is sufficient.

agis · 2018-04-17T09:22:18Z

job.go

+// State determines the job's current state by using it's path in the filesystem.
+func (j *Job) State(path, project, id string) (string, error) {
+	pPath := filepath.Join(path, project, "pending", id)
+	rPath := filepath.Join(path, project, "ready", id)


Can't we use j.PendingBuildPath and j.ReadyBuildPath?

I don't think we can as the job has been initialized with an empty struct

j := &Job{} state, err := j.GetState(s.cfg.BuildPath, project, id)

Probably we can introduce a GetJob function that would find a job, WDYT?

Didn't notice that. Jobs should not be initialized this way, we should only use NewJob().

agis · 2018-04-17T09:22:46Z

public/templates/show.html

+
+  <div class="top-bar">
+    <div class="top-bar-left">
+      <h1><a href="/">Mistry</a></h1>


agis · 2018-04-17T09:25:57Z

server_test.go

@@ -65,3 +72,87 @@ func TestLoad(t *testing.T) {
 		<-results
 	}
 }
+
+func TestIndex(t *testing.T) {


Tests functions are named according to the functions they test. This should be named TestHandleIndex for example.

agis · 2018-04-17T09:26:02Z

server_test.go

+	}
+}
+
+func TestShow(t *testing.T) {


Ditto here.

agis · 2018-04-17T09:26:18Z

server_test.go

+}
+
+func TestShow(t *testing.T) {
+	// Build a job.


agis · 2018-04-17T09:28:40Z

types/build_result.go

@@ -23,6 +24,9 @@ type BuildResult struct {

 	// The method by which the build artifacts can be fetched.
 	TransportMethod TransportMethod
+
+	// The job's start time
+	StartTime time.Time


This belongs in Job.

Moved to Job.

agis · 2018-04-17T09:29:07Z

types/job_info.go

+	Output    string
+	Log       string
+	State     string
+}


agis · 2018-04-17T09:30:17Z

Please also add a small section in the README on where the web view can be found.

agis · 2018-04-17T15:31:38Z

job.go

+func (j *Job) State(path, project, id string) (string, error) {
+	pPath := filepath.Join(path, project, "pending", id)
+	rPath := filepath.Join(path, project, "ready", id)
+	_, err := os.Stat(pPath)


Instead for checking through the filesystem, which is racy if other methods of mistry manipulate the same paths, you can use the JobQueue struct which the server already contains.

Good catch, but while I agree that this is racy, what would happen when we restart the server?The JobQueue would be empty then and the index page wouldn't display any jobs even if there are some. That's why I believe we should scan the filesystem.

Let's revisit this at the next iteration of the web view. It's fine for now.

agis · 2018-04-17T15:39:36Z

We should add a new issue tracking what we need to revamp in the implementation.

apostergiou · 2018-04-19T10:50:09Z

We should add a new issue tracking what we need to revamp in the implementation.

Tracked on #38.

Also extend job struct and add JSON encodings

Tailer provides a tailf functionality for reading from continuously updated files.

The broker package implements a registry for open client connections and functions that enable listening and broadcasting events for the registered clients.

This generally makes sense but also assists us in web view.

apostergiou force-pushed the web-view branch 2 times, most recently from ff75cdc to 9d080f0 Compare April 6, 2018 09:28

apostergiou requested a review from agis April 6, 2018 09:32

apostergiou self-assigned this Apr 6, 2018

apostergiou added the feature label Apr 6, 2018

agis suggested changes Apr 6, 2018

View reviewed changes

agis added this to the 1.0 milestone Apr 12, 2018

apostergiou force-pushed the web-view branch 2 times, most recently from 0d60114 to 9569b0f Compare April 17, 2018 08:20

agis suggested changes Apr 17, 2018

View reviewed changes

apostergiou force-pushed the web-view branch from 9569b0f to 9dd198e Compare April 18, 2018 08:14

agis force-pushed the web-view branch from 8b8a8eb to 9ccff42 Compare April 18, 2018 10:54

apostergiou force-pushed the web-view branch from 9ccff42 to 24afdcb Compare April 18, 2018 11:53

agis force-pushed the web-view branch from 24afdcb to 0123357 Compare April 18, 2018 12:11

apostergiou force-pushed the web-view branch from a85e73a to 8217bee Compare April 19, 2018 10:49

apostergiou and others added 9 commits April 19, 2018 14:11

job: Add helpers for web view

f23a3c1

Also extend job struct and add JSON encodings

Introduce tailer package

f9801b7

Tailer provides a tailf functionality for reading from continuously updated files.

Introduce broker package

4877f93

The broker package implements a registry for open client connections and functions that enable listening and broadcasting events for the registered clients.

Add foundation files

91a82f1

Web view: Implement index page

117ba08

Web view: Implement show page

8045e86

README: Update web view information

eabbcd8

Add job parameters to BuildResult

c23af5e

This generally makes sense but also assists us in web view.

webview: Properly output docker build log

0deaac7

agis force-pushed the web-view branch from b386440 to e86dc70 Compare April 19, 2018 13:02

agis approved these changes Apr 19, 2018

View reviewed changes

agis added 2 commits April 19, 2018 16:30

Embed web view templates to the binary

d3868dc

Various field renames

a2ac779

apostergiou force-pushed the web-view branch from ab9ef90 to a2ac779 Compare April 19, 2018 13:32

apostergiou merged commit 5595bf9 into master Apr 19, 2018

apostergiou deleted the web-view branch April 19, 2018 13:35

agis added a commit that referenced this pull request Apr 19, 2018

Update changelog due to #17

f012348

Add a Web view #17

Add a Web view #17

Conversation

apostergiou commented Apr 6, 2018

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

apostergiou Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

apostergiou commented Apr 6, 2018 • edited Loading

agis commented Apr 12, 2018

agis 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

agis Apr 17, 2018 • edited Loading

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

agis commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agis commented Apr 17, 2018

apostergiou commented Apr 19, 2018

apostergiou Apr 6, 2018 •

edited

Loading

apostergiou commented Apr 6, 2018 •

edited

Loading

agis Apr 17, 2018 •

edited

Loading