[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Web interface pushed

From: Amirouche Boubekki
Subject: Re: Web interface pushed
Date: Thu, 2 Aug 2018 20:13:28 +0200

I read some of the code, I like it :)

diff --git a/src/cuirass/base.scm b/src/cuirass/base.scm
index 82f49a4..af24a28 100644
--- a/src/cuirass/base.scm
+++ b/src/cuirass/base.scm
@@ -20,6 +20,10 @@
 ;;; You should have received a copy of the GNU General Public License
 ;;; along with Cuirass.  If not, see <>.
+;; Comment:
+;; This is called base because it interface with guix daemon.
 (define-module (cuirass base)
   #:use-module (fibers)
   #:use-module (cuirass logging)
diff --git a/src/cuirass/http.scm b/src/cuirass/http.scm
index 2d66ff9..0899cd1 100644
--- a/src/cuirass/http.scm
+++ b/src/cuirass/http.scm
@@ -117,6 +117,13 @@
     (map build->hydra-build builds)))
 (define (request-parameters request)
+  ;; REVIEW: this 'parameters' is ambigious in aiohttp. It's called
+  ;; request.query because the part after ? in the URL is called the "query
+  ;; string". Also parameters can be passed via the path part of the URL.
+  ;; here is what I use
+  ;;
   "Parse the REQUEST query parameters and return them under the form
   '((parameter . value) ...)."
   (let* ((uri (request-uri request))
@@ -125,11 +132,14 @@
         (map (lambda (param)
                (match (string-split param #\=)
                  ((key param)
-                  (let ((key-symbol (string->symbol key)))
+                  (let ((key-symbol (string->symbol (uri-decode key))))
                     (cons key-symbol
                           (match key-symbol
-                            ('id (string->number param))
-                            ('nr (string->number param))
+                                 ('id (string->number (uri-decode param)))
+                                 ;; I don't think this will scale in terms of
+                                 ;; kind query pairs where the key part can be
+                                 ;; many different things
+                            ('nr (string->number (uri-decode param)))
                             (_   param)))))))
              (string-split query #\&))
@@ -138,9 +148,10 @@
 ;;; Web server.
-;;; The api is derived from the hydra one. It is partially described here :
+;;; The exposed api is derived from the hydra one. It is partially described
+;;; here :
 (define (request-path-components request)
@@ -217,6 +228,8 @@
                     (with-critical-section db-channel (db)
                       (db-get-specifications db)))))
     (("build" build-id)
+     ;; REVIEW: don't inline that much code in same procedure aka. procedures
+     ;; for the win1
      (let ((hydra-build
             (with-critical-section db-channel (db)
               (handle-build-request db (string->number build-id)))))
@@ -234,6 +247,7 @@
               (let ((uri (string->uri-reference
                           (string-append "/log/"
                                          (basename output)))))
+                ;; create a procedure for that when you will have forms.
                 (respond (build-response #:code 302
                                          #:headers `((location . ,uri)))
                          #:body "")))
@@ -241,7 +255,10 @@
               ;; Not entry for BUILD-ID in the 'Outputs' table.
+               ;; 500 is very strong error, it's must not be used for expected failure
+               ;; by the way this is rendering logic, it should be in it's own procedure.
                (format #f "Outputs of build ~a are unknown." build-id)))
               (respond-build-not-found build-id)))
            (respond-build-not-found build-id))))
@@ -251,13 +268,25 @@
             (nr (assq-ref params 'nr)))
        (if nr
            (respond-json (object->json-string
+                          ;; don't nest here the code. Because http handlers should follow
+                          ;; the following pattern:
+                          ;;
+                          ;;   (define (handler request body)
+                          ;;      (let ((input (snarf-input request body)))
+                          ;;          (shallow-validate-with-exception input)
+                          ;;          (deep-validate-with-exception input (db))
+                          ;;          (let ((out (domain-code-goes-here)))
+                          ;;             (sxml->response (handler-template out))))
+                          ;;
+                          ;; Basically, the current code requires top down and bottom up
+                          ;; reading. Declaring intermediate variables helps readbility.
                           (with-critical-section db-channel (db)
                             (db-get-evaluations db nr))))
-           (respond-json-with-error 500 "Parameter not defined!"))))
+           (respond-json-with-error 400 "Parameter not defined!"))))  ;; aka. bad request
     (("api" "latestbuilds")
      (let* ((params (request-parameters request))
             ;; 'nr parameter is mandatory to limit query size.
-            (valid-params? (assq-ref params 'nr)))
+            (valid-params? (assq-ref params 'nr)))  ;; or exception?
        (if valid-params?
            ;; Limit results to builds that are "done".
@@ -359,7 +388,10 @@
     ;; process each client request and then directly go back waiting for the
     ;; next client (conversely, Guile's 'run-server' loop processes clients
     ;; one after another, sequentially.)  We can do that because we don't
-    ;; maintain any state across connections.
+    ;; maintain any state across connections.  I don't understand that
+    ;; comment. Why no state across connections? isn't sqlite that stores
+    ;; states? Does it mean there is no global mutable in memory data in
+    ;; cuirass?
     ;; XXX: We don't do 'call-with-sigint' like 'run-server' does.
     (let* ((impl (lookup-server-impl 'fiberized))
diff --git a/src/schema.sql b/src/schema.sql
index eb0f7e9..769360f 100644
--- a/src/schema.sql
+++ b/src/schema.sql
@@ -1,5 +1,8 @@
+-- REVIEW: The name of the table must be all small caps.
+-- COMMENT: Some people argue table name must be singular
+--          but I disagree, both might work.
 CREATE TABLE Specifications (
   name          TEXT NOT NULL PRIMARY KEY,
   load_path_inputs TEXT NOT NULL, -- list of input names whose load path will be in Guile's %load-path
@@ -64,7 +67,7 @@ CREATE TABLE Builds (
   log           TEXT NOT NULL,
   status        INTEGER NOT NULL,
   timestamp     INTEGER NOT NULL,
-  starttime     INTEGER NOT NULL,
+  starttime     INTEGER NOT NULL,  -- REVIEW: why not start_time?
   stoptime      INTEGER NOT NULL,
   FOREIGN KEY (derivation) REFERENCES Derivations (derivation),
   FOREIGN KEY (evaluation) REFERENCES Evaluations (id)
@@ -75,5 +78,12 @@ CREATE TABLE Builds (
 CREATE INDEX Builds_Derivations_index ON Builds(status ASC, timestamp ASC, id, derivation, evaluation, stoptime DESC);
 CREATE INDEX Inputs_index ON Inputs(specification, name, branch);
 CREATE INDEX Derivations_index ON Derivations(derivation, evaluation, job_name, system);
+-- COMMENT: Indices make the following trade off: slow down writes and take
+--          more space (disk and memory) and in prize you get faster reads.
+-- COMMENT: you'd better add CREATE INDEX just below the table that they
+--          speed up queries for.

Le mer. 1 août 2018 à 21:47, Tatiana Sholokhova <address@hidden> a écrit :
Hi all,

Thank you for the congratulations! I am excited to have the web-interface pushed into the master branch. I would like to thank you all for your great support. Special thanks to Clément for helpful final reviews and merging all the changes together.

Now I would like to add some more features to the interface to enhance it further. Am I right that now I should again organize my changes as a new separate branch?

Best regards,

On Mon, 30 Jul 2018 at 15:27, Ricardo Wurmus <address@hidden> wrote:

Hi Pjotr,

> One comment: I think we should strive to design GSoC programmes in a
> way that students get to push earlier to trunk. I know Guix can be
> complex, but even so, I think we would have less failure if we make it
> small pieces and better gratification.

This has always been our goal.  The projects can usually be implemented
in independent chunks that could be merged into the “master” branch at
various stages.


Attachment: cuirass.amz3.20180802.diff
Description: Text Data

reply via email to

[Prev in Thread] Current Thread [Next in Thread]