Include skipped builds in database and web ui

We have had an on-and-off relationship with skipped builds in the
database.  Generally we have attempted to exclude them from the db,
but we have occasionally (accidentally?) included them.  The status
quo is that builds with a result of SKIPPED (as well as several
other results which don't come from the executor) are not recorded
in the database.

With a greater interest in being able to determine which jobs ran
or did not run for a change after the fact, this job deliberately
adds all builds (whether they touch an executor or not, whether
real or not) to the database.  This means than anything that could
potentially show up on the status page or in a code-review report
will be in the database, and can therefore be seen in the web UI.

It is still the case that we are not actually interested in seeing
a page full of SKIPPED builds when we visit the "Builds" tab in
the web ui (which is the principal reason we have not included them
in the database so far).  To address this, we set the default query
in the builds tab to exclude skipped builds (it is easy to add other
types of builds to exclude in the future if we wish).  If a user
then specifies a query filter to *include* specific results, we drop
the exclusion from the query string.  This allows for the expected
behavior of not showing SKIPPED by default, then as specific results
are added to the filter, we show only those, and if the user selects
that they want to see SKIPPED, they will then be included.

On the buildset page, we add a switch similar to the current "show
retried jobs" switch that selects whether skipped builds in a buildset
should be displayed (again, it hides them by default).

Change-Id: I1835965101299bc7a95c952e99f6b0b095def085

7 files changed, 112 insertions, 13 deletions

diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py
index d8901fabb..dab60efc6 100644
--- a/tests/unit/test_web.py
+++ b/tests/unit/test_web.py
@@ -1783,6 +1783,45 @@ class TestBuildInfo(BaseTestWeb):
                                     "idx_min=%i" % idx_max).json()
         self.assertEqual(len(builds_query), 1, builds_query)
 
+    def test_web_list_skipped_builds(self):
+        # Test the exclude_result filter
+        # Generate some build records in the db.
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        self.executor_server.failJob('project-merge', A)
+        A.addApproval('Code-Review', 2)
+        self.fake_gerrit.addEvent(A.addApproval('Approved', 1))
+        self.waitUntilSettled()
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        builds = self.get_url("api/tenant/tenant-one/builds").json()
+        builds.sort(key=lambda x: x['job_name'])
+        self.assertEqual(len(builds), 3)
+        self.assertEqual(builds[0]['job_name'], 'project-merge')
+        self.assertEqual(builds[1]['job_name'], 'project-test1')
+        self.assertEqual(builds[2]['job_name'], 'project-test2')
+        self.assertEqual(builds[0]['result'], 'FAILURE')
+        self.assertEqual(builds[1]['result'], 'SKIPPED')
+        self.assertEqual(builds[2]['result'], 'SKIPPED')
+
+        builds = self.get_url("api/tenant/tenant-one/builds?"
+                              "exclude_result=SKIPPED").json()
+        self.assertEqual(len(builds), 1)
+        self.assertEqual(builds[0]['job_name'], 'project-merge')
+        self.assertEqual(builds[0]['result'], 'FAILURE')
+
+        builds = self.get_url("api/tenant/tenant-one/builds?"
+                              "result=SKIPPED&result=FAILURE").json()
+        builds.sort(key=lambda x: x['job_name'])
+        self.assertEqual(len(builds), 3)
+        self.assertEqual(builds[0]['job_name'], 'project-merge')
+        self.assertEqual(builds[1]['job_name'], 'project-test1')
+        self.assertEqual(builds[2]['job_name'], 'project-test2')
+        self.assertEqual(builds[0]['result'], 'FAILURE')
+        self.assertEqual(builds[1]['result'], 'SKIPPED')
+        self.assertEqual(builds[2]['result'], 'SKIPPED')
+
     def test_web_badge(self):
         # Generate some build records in the db.
         self.add_base_changes()
diff --git a/web/src/containers/FilterToolbar.jsx b/web/src/containers/FilterToolbar.jsx
index 9568997cc..4cc1e3f25 100644
--- a/web/src/containers/FilterToolbar.jsx
+++ b/web/src/containers/FilterToolbar.jsx
@@ -319,14 +319,21 @@ function writeFiltersToUrl(filters, location, history) {
 
 function buildQueryString(filters) {
   let queryString = '&complete=true'
+  let resultFilter = false
   if (filters) {
     Object.keys(filters).map((key) => {
       filters[key].forEach((value) => {
+        if (value === 'result') {
+          resultFilter = true
+        }
         queryString += '&' + key + '=' + value
       })
       return queryString
     })
   }
+  if (!resultFilter) {
+      queryString += '&exclude_result=SKIPPED'
+  }
   return queryString
 }
 
diff --git a/web/src/containers/build/BuildList.jsx b/web/src/containers/build/BuildList.jsx
index 69f1795bf..ccb734679 100644
--- a/web/src/containers/build/BuildList.jsx
+++ b/web/src/containers/build/BuildList.jsx
@@ -57,15 +57,24 @@ class BuildList extends React.Component {
       return self.indexOf(build) === idx
     })
 
+    let skippedJobs = builds.filter((build) => {
+      return build.result === 'SKIPPED'
+    }).map((build) => (build.job_name)
+    ).filter((build, idx, self) => {
+      return self.indexOf(build) === idx
+    })
+
     this.state = {
       visibleNonFinalBuilds: retriedJobs,
       retriedJobs: retriedJobs,
+      skippedJobs: skippedJobs,
+      showSkipped: false,
     }
   }
 
   sortedBuilds = () => {
     const { builds } = this.props
-    const { visibleNonFinalBuilds } = this.state
+    const { visibleNonFinalBuilds, showSkipped } = this.state
 
     return builds.sort((a, b) => {
       // Group builds by job name, then order by decreasing start time; this will ensure retries are together
@@ -85,6 +94,9 @@ class BuildList extends React.Component {
       }
     }).filter((build) => {
       if (build.final || visibleNonFinalBuilds.indexOf(build.job_name) >= 0) {
+        if (build.result === 'SKIPPED' && !showSkipped) {
+          return false
+        }
         return true
       }
       else {
@@ -98,6 +110,10 @@ class BuildList extends React.Component {
     this.setState({ visibleNonFinalBuilds: (isChecked ? retriedJobs : []) })
   }
 
+  handleSkippedSwitch = isChecked => {
+    this.setState({ showSkipped: isChecked })
+  }
+
   handleToggleVisibleNonFinalBuilds = (jobName) => {
     const { visibleNonFinalBuilds } = this.state
     const index = visibleNonFinalBuilds.indexOf(jobName)
@@ -138,7 +154,7 @@ class BuildList extends React.Component {
 
   render() {
     const { tenant } = this.props
-    const { visibleNonFinalBuilds, retriedJobs } = this.state
+    const { visibleNonFinalBuilds, retriedJobs, skippedJobs, showSkipped } = this.state
 
     let retrySwitch = retriedJobs.length > 0 ?
       <FlexItem align={{ default: 'alignRight' }}>
@@ -151,10 +167,22 @@ class BuildList extends React.Component {
       </FlexItem> :
       <></>
 
+    let skippedSwitch = skippedJobs.length > 0 ?
+      <FlexItem align={{ default: 'alignRight' }}>
+        <span>Show skipped jobs &nbsp;</span>
+        <Switch
+          isChecked={showSkipped}
+          onChange={this.handleSkippedSwitch}
+          isReversed
+        />
+      </FlexItem> :
+      <></>
+
     const sortedBuilds = this.sortedBuilds()
 
     return (
       <Flex direction={{ default: 'column' }}>
+        {skippedSwitch}
         {retrySwitch}
         <FlexItem>
           <DataList
diff --git a/zuul/driver/sql/sqlconnection.py b/zuul/driver/sql/sqlconnection.py
index 1af50b2f7..8ab528c39 100644
--- a/zuul/driver/sql/sqlconnection.py
+++ b/zuul/driver/sql/sqlconnection.py
@@ -59,6 +59,14 @@ class DatabaseSession(object):
             return query.filter(column.in_(value))
         return query.filter(column == value)
 
+    def exListFilter(self, query, column, value):
+        # Exclude values in list
+        if value is None:
+            return query
+        if isinstance(value, list) or isinstance(value, tuple):
+            return query.filter(column.not_in(value))
+        return query.filter(column != value)
+
     def getBuilds(self, tenant=None, project=None, pipeline=None,
                   change=None, branch=None, patchset=None, ref=None,
                   newrev=None, event_id=None, event_timestamp=None,
@@ -66,7 +74,8 @@ class DatabaseSession(object):
                   uuid=None, job_name=None, voting=None, nodeset=None,
                   result=None, provides=None, final=None, held=None,
                   complete=None, sort_by_buildset=False, limit=50,
-                  offset=0, idx_min=None, idx_max=None):
+                  offset=0, idx_min=None, idx_max=None,
+                  exclude_result=None):
 
         build_table = self.connection.zuul_build_table
         buildset_table = self.connection.zuul_buildset_table
@@ -114,6 +123,7 @@ class DatabaseSession(object):
         q = self.listFilter(q, build_table.c.voting, voting)
         q = self.listFilter(q, build_table.c.nodeset, nodeset)
         q = self.listFilter(q, build_table.c.result, result)
+        q = self.exListFilter(q, build_table.c.result, exclude_result)
         q = self.listFilter(q, build_table.c.final, final)
         if complete is True:
             q = q.filter(build_table.c.result != None)  # noqa
diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py
index d70b12885..19e141dba 100644
--- a/zuul/manager/__init__.py
+++ b/zuul/manager/__init__.py
@@ -1965,13 +1965,7 @@ class PipelineManager(metaclass=ABCMeta):
             build_set.jobNodeRequestComplete(request.job_name, nodeset)
         # Put a fake build through the cycle to clean it up.
         if not request.fulfilled:
-            fakebuild = build_set.item.setNodeRequestFailure(job)
-            try:
-                self.sql.reportBuildEnd(
-                    fakebuild, tenant=build_set.item.pipeline.tenant.name,
-                    final=True)
-            except Exception:
-                log.exception("Error reporting build completion to DB:")
+            build_set.item.setNodeRequestFailure(job)
             self._resumeBuilds(build_set)
             tenant = build_set.item.pipeline.tenant
             tenant.semaphore_handler.release(
diff --git a/zuul/model.py b/zuul/model.py
index 67b5e0117..8e794cea3 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -4921,6 +4921,10 @@ class QueueItem(zkobject.ZKObject):
                                   job=job, build_set=self.current_build_set,
                                   result='FAILURE')
             self.addBuild(fakebuild)
+            self.pipeline.manager.sql.reportBuildEnd(
+                fakebuild,
+                tenant=self.pipeline.tenant.name,
+                final=True)
             self.setResult(fakebuild)
             ret = False
         return ret
@@ -5254,6 +5258,10 @@ class QueueItem(zkobject.ZKObject):
                                       build_set=self.current_build_set,
                                       result='SKIPPED')
                 self.addBuild(fakebuild)
+                self.pipeline.manager.sql.reportBuildEnd(
+                    fakebuild,
+                    tenant=self.pipeline.tenant.name,
+                    final=True)
 
     def setNodeRequestFailure(self, job):
         fakebuild = Build.new(
@@ -5265,8 +5273,11 @@ class QueueItem(zkobject.ZKObject):
             result='NODE_FAILURE',
         )
         self.addBuild(fakebuild)
+        self.pipeline.manager.sql.reportBuildEnd(
+            fakebuild,
+            tenant=self.pipeline.tenant.name,
+            final=True)
         self.setResult(fakebuild)
-        return fakebuild
 
     def setDequeuedNeedingChange(self, msg):
         self.updateAttributes(
@@ -5322,6 +5333,10 @@ class QueueItem(zkobject.ZKObject):
                                   job=job, build_set=self.current_build_set,
                                   result='SKIPPED')
             self.addBuild(fakebuild)
+            self.pipeline.manager.sql.reportBuildEnd(
+                fakebuild,
+                tenant=self.pipeline.tenant.name,
+                final=True)
 
     def _setMissingJobsSkipped(self):
         for job in self.getJobs():
@@ -5332,6 +5347,10 @@ class QueueItem(zkobject.ZKObject):
                                   job=job, build_set=self.current_build_set,
                                   result='SKIPPED')
             self.addBuild(fakebuild)
+            self.pipeline.manager.sql.reportBuildEnd(
+                fakebuild,
+                tenant=self.pipeline.tenant.name,
+                final=True)
 
     def getNodePriority(self):
         return self.pipeline.manager.getNodePriority(self)
diff --git a/zuul/web/__init__.py b/zuul/web/__init__.py
index 995b28271..6da1ae303 100755
--- a/zuul/web/__init__.py
+++ b/zuul/web/__init__.py
@@ -1399,7 +1399,8 @@ class ZuulWebAPI(object):
                branch=None, patchset=None, ref=None, newrev=None,
                uuid=None, job_name=None, voting=None, nodeset=None,
                result=None, final=None, held=None, complete=None,
-               limit=50, skip=0, idx_min=None, idx_max=None):
+               limit=50, skip=0, idx_min=None, idx_max=None,
+               exclude_result=None):
         connection = self._get_connection()
 
         if tenant not in self.zuulweb.abide.tenants.keys():
@@ -1423,7 +1424,8 @@ class ZuulWebAPI(object):
             branch=branch, patchset=patchset, ref=ref, newrev=newrev,
             uuid=uuid, job_name=job_name, voting=voting, nodeset=nodeset,
             result=result, final=final, held=held, complete=complete,
-            limit=limit, offset=skip, idx_min=_idx_min, idx_max=_idx_max)
+            limit=limit, offset=skip, idx_min=_idx_min, idx_max=_idx_max,
+            exclude_result=exclude_result)
 
         resp = cherrypy.response
         resp.headers['Access-Control-Allow-Origin'] = '*'