Add the job start time to build info file #47

apostergiou · 2018-04-20T13:58:27Z

Rename buildResult to buildInfo
Add start time to build info
work: Write build info file before container start
web view index: Sort jobs by started time on index page web view: Sort jobs by started time on index page #42
web view show: get job start time from the build info file

agis · 2018-04-23T07:09:05Z

cmd/mistryd/mistryd_test.go

@@ -151,7 +151,7 @@ func TestPruneZombieBuilds(t *testing.T) {
 	}
 }

-func readOut(br *types.BuildResult, path string) (string, error) {
+func readOut(br *types.BuildInfo, path string) (string, error) {


br -> bi

agis · 2018-04-23T07:11:05Z

cmd/mistryd/worker.go

+		return
+	}
+
+	infoFile.Seek(0, 0)


We should always check & handle errors.

good catch, fixed

agis · 2018-04-23T07:24:04Z

cmd/mistryd/worker.go

+
+	err = j.BuildImage(ctx, s.cfg.UID, client, out)
+	if err != nil {
+		return


Just noticed that we do not set any errors here. We should fix this at this PR.

agis · 2018-04-23T07:27:00Z

cmd/mistryd/job.go

@@ -118,6 +118,8 @@ func NewJob(project string, params types.Params, group string, cfg *Config) (*Jo
 	j.Image = ImgCntPrefix + j.ID
 	j.Container = ImgCntPrefix + j.ID

+	j.StartedAt = time.Now()


StartedAt is not needed in Job{}. It should only exist in BuildInfo{}.

True, we only need this for the index sort. Nevertheless, I say we take care of this in another PR as it affects the server and the job JSON encoding. Opened a new issue to track it #49.

agis · 2018-04-23T07:28:13Z

cmd/mistryd/server.go

 	if err != nil {
 		s.Log.Print(err)
 		w.WriteHeader(http.StatusInternalServerError)
 		return
 	}

-	jDir, err := ioutil.ReadDir(jPath)
+	buildLog, err = ioutil.ReadFile(buildLogPath)


[minor] IMHO log is a better name

agis · 2018-04-23T07:30:49Z

cmd/mistryd/server.go

+				w.WriteHeader(http.StatusInternalServerError)
+				return
+			}
+			err = json.Unmarshal(biBlob, &bi)


What if err is not nil?

True, handled.

agis · 2018-04-23T07:31:19Z

cmd/mistryd/server.go

+				w.WriteHeader(http.StatusInternalServerError)
+				return
+			}
+			err = json.Unmarshal(biBlob, &bi)


agis · 2018-04-23T07:32:37Z

cmd/mistryd/server.go

 			jobs = append(jobs, Job{
 				ID:        j.Name(),
 				Project:   p.Name(),
-				StartedAt: j.ModTime(),
-				Output:    filepath.Join(pendingPath, j.Name(), BuildLogFname),
+				StartedAt: bi.StartedAt,
 				State:     "pending"})
 		}

 		for _, j := range readyJobs {


The body of this loop is the exact same with the one some lines above. Can we somehow get rid of this repetition?

Introduced a GetJobs function and extracted the repetitive code to a buildInfo function.

agis · 2018-04-23T07:33:44Z

cmd/mistry/Gopkg.lock

@@ -1,12 +1,6 @@
 # This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'.


Use BuildResult (as opposed to buildResult) and BuildInfo in the commit message.

agis · 2018-04-23T11:47:55Z

cmd/mistryd/job.go

@@ -275,3 +278,51 @@ func GetState(path, project, id string) (string, error) {
 	}
 	return "", fmt.Errorf("job with id=%s not found error", id)
 }
+
+func GetJobs(path string) ([]Job, error) {


Add documentation. Also, this should be unexported.

This belongs to the server.

agis · 2018-04-23T11:53:18Z

cmd/mistryd/job.go

+				ID:        j.Name(),
+				Project:   p.Name(),
+				StartedAt: bi.StartedAt,
+				State:     "ready"})


The repetition here still remains. You do the exact same things for pendingJobs and readyJobs. We could DRY this.

agis · 2018-04-24T07:44:27Z

pkg/types/build_info.go

@@ -36,3 +41,16 @@ type ErrImageBuild struct {
 func (e ErrImageBuild) Error() string {
 	return fmt.Sprintf("could not build docker image '%s': %s", e.Image, e.Err)
 }
+
+func GetBuildInfo(path string) (BuildInfo, error) {


This function doesn't belong in this package. I'd say to not make this a function at all, since it's only used in a single place inside the server.

(Since it was exported it should also have documentation).

True, since we refactored the getJob to work as a closure, we can delete the extra function.

agis · 2018-04-24T07:45:29Z

cmd/mistryd/public/templates/show.html

-      jobInfo.innerHTML += "ExitCode: ".big() + jobJSON["ExitCode"] + "<br>";
-      jobInfo.innerHTML += "Transport method: ".big() + jobJSON["TransportMethod"] + "<br>";
-    }
+    jobInfo.innerHTML += "Started At: ".big() + jobJSON["StartedAt"] + "<br>";


Can we parse this and return a more human representation of the date? Eg. DD-MM-YYYY 12:34:123 +300

agis · 2018-04-24T07:47:50Z

cmd/mistryd/server.go

+	for _, p := range projects {
+		pendingPath := filepath.Join(s.cfg.BuildPath, p.Name(), "pending")
+		_, err := os.Stat(pendingPath)
+		pendingExists := !os.IsNotExist(err)


What if err is not nil and not a "not exist" error?

agis · 2018-04-24T07:47:58Z

cmd/mistryd/server.go

+		pendingExists := !os.IsNotExist(err)
+		readyPath := filepath.Join(s.cfg.BuildPath, p.Name(), "ready")
+		_, err = os.Stat(readyPath)
+		readyExists := !os.IsNotExist(err)


agis · 2018-04-24T07:49:35Z

cmd/mistryd/server.go

+		Log:     template.HTML(strings.Replace(string(log), "\n", "<br />", -1)),
+		ID:      id,
+		Project: project,
+		State:   state,


Don't we need StartedAt here?

No, the view parses the output field. In addition, since we want to remove the StartedAt from the job, I suggest we leave this be.

agis · 2018-04-24T07:51:50Z

cmd/mistryd/server.go

+				return nil, fmt.Errorf("cannot find job %s; %s", j.Name(), err)
+			}
+			jobs = append(jobs, job)
+		}


You could get rid of the 2nd loop (it is identical to 1st one), if you concatenate the two slices:

for _, j := range append(pendingJobs, readyJobs...) // ...

These loops are not identical.

Fixes #42.

Fixes #50.

apostergiou changed the title ~~[wip] Fix dir modtime~~ Fix dir modtime Apr 23, 2018

apostergiou force-pushed the fix-dir-modtime branch from a3e98b5 to 3ffba9a Compare April 23, 2018 07:03

apostergiou changed the title ~~Fix dir modtime~~ Add the job start time to build info file Apr 23, 2018

apostergiou requested review from agis and dtheodor April 23, 2018 07:03

apostergiou added enhancement server labels Apr 23, 2018

apostergiou self-assigned this Apr 23, 2018

apostergiou force-pushed the fix-dir-modtime branch from 3ffba9a to f85d068 Compare April 23, 2018 07:04

apostergiou added this to the 1.0 milestone Apr 23, 2018

agis suggested changes Apr 23, 2018

View reviewed changes

apostergiou mentioned this pull request Apr 23, 2018

Remove BuildInfo.StartedAt #49

Open

agis added bug Something isn't working and removed enhancement labels Apr 23, 2018

apostergiou force-pushed the fix-dir-modtime branch from f85d068 to 9175c2b Compare April 23, 2018 08:56

agis mentioned this pull request Apr 23, 2018

Do not return error if pending/ready has not been created yet #50

Closed

agis suggested changes Apr 23, 2018

View reviewed changes

apostergiou force-pushed the fix-dir-modtime branch 2 times, most recently from 6743d79 to 0c6a214 Compare April 24, 2018 06:11

agis suggested changes Apr 24, 2018

View reviewed changes

agis approved these changes Apr 25, 2018

View reviewed changes

apostergiou force-pushed the fix-dir-modtime branch from bbbfef6 to bf34ea9 Compare April 25, 2018 10:25

apostergiou added 8 commits April 25, 2018 13:36

types: Rename BuildResult to BuildInfo

85a0f12

Add start time to build info

c30010f

work: Write build info file before container start

271ddff

view: Fetch started at from build info file

edba2e9

view: Use the build_info time to sort jobs

8f1b038

Fixes #42.

Refactor server index to use getJobs

eaccf36

Fixes #50.

work: Set docker build image errors

65297e9

Update dep lockfile

dd45d61

apostergiou force-pushed the fix-dir-modtime branch from 80e4bcf to dd45d61 Compare April 25, 2018 10:37

apostergiou merged commit dd45d61 into master Apr 25, 2018

apostergiou deleted the fix-dir-modtime branch April 25, 2018 11:25

		@@ -1,12 +1,6 @@
		# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'.

Add the job start time to build info file #47

Add the job start time to build info file #47

Conversation

apostergiou commented Apr 20, 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

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 25, 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

apostergiou commented Apr 20, 2018 •

edited

Loading

apostergiou Apr 25, 2018 •

edited

Loading